Inventory rebased - #16
Closed
marcinpsk wants to merge 32 commits into
Closed
Conversation
Added a pull request template to standardize PR submissions.
develop to master release prep
Develop to master
release: bump version to 0.4.3 and update changelog
- Store librenms_id as {server_key: device_id} dict instead of bare int
- Add get_librenms_device_id/set_librenms_device_id/find_by_librenms_id/
migrate_legacy_librenms_id helpers in utils.py (at end of file)
- Thread server_key through import pipeline (filters, cache, bulk_import,
device_operations, vm_operations, virtual_chassis)
- Deterministic SHA256 cache keys, None-safe filter inclusion
- Fix disabled field filter (use 'disabled' flag, not 'status')
- Truthy string parsing for use_sysname/strip_domain ('on'/'true'/'1')
- migrate_librenms_id action in DeviceConflictActionView
- RemoveServerMappingView for per-server librenms_id removal
- New tests: test_permissions.py, test_sync_view_mismatch.py
- RQ-based job cancellation check in bulk import
Reduce cosmetic diff vs develop:
- _save_device restored to before _resolve_naming_preferences in actions.py
- _empty_return moved to just before process_device_filters in bulk_import.py
- New utils.py helpers at end of file with imports merged into main block
Add ENTITY-MIB inventory sync for device modules (module types, module bays, transceivers, normalization rules) on top of the librenms_id JSON migration. Features added: - Module bay sync from LibreNMS ENTITY-MIB inventory data - ModuleBayMapping model for LibreNMS to NetBox bay name translation - ModuleTypeMapping for hardware model normalization - DeviceTypeMapping for device hardware matching - NormalizationRule model for generic regex-based string normalization - Module sync views (BaseModuleTableView, object sync tabs) - Transceiver detection via LibreNMS port transceiver API - Virtual chassis module support with per-member inventory - New migrations (0009-0013) for new models - New contrib/ YAML examples for mappings and rules - E2E test scaffolding in tests/e2e/
When a converter bay is matched but not yet installed in NetBox,
reset bays_by_depth[depth+1] to {} to prevent the children of a
subsequent uninstalled sibling from inheriting the bay scope of the
previously processed installed sibling.
Also adds tests for:
- test_modules_view.py: regression tests for bay-scope/serial-mismatch bug
- TestSetLibreNMSDeviceId (test_utils.py): set_librenms_device_id coercion
- TestSafeDisabledBulkImport / TestSafeDisabledFilters: bool/string handling
- TestVCPositionHandling: update_vc_member_suggested_names no off-by-one,
position preservation, and zero-position fallback
- TestNameMatchesWithNamingPreferences: naming_criteria source falls back
to 'sysname' when hostname is empty
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generate HTML coverage report as build artifact on Python 3.12 runs. No threshold gating - informational only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds mapping models/migrations, per-server librenms_id management and migration, extensive import/cache/VC/name-resolution and module-sync flows (including transactional module installs), new API endpoints/serializers/forms/tables/templates, broad tests (unit/integration/e2e), and CI/devcontainer/docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeviceView as Device Sync View
participant LibreAPI as LibreNMS API
participant Cache as Cache Layer
participant DB as NetBox DB
participant Match as Mapping/Normalization
User->>DeviceView: POST Refresh Modules
DeviceView->>LibreAPI: GET /devices/{id}/inventory and /transceivers
LibreAPI-->>DeviceView: Inventory + Transceivers
DeviceView->>Match: Query ModuleBay/ModuleType/NormalizationRule
Match->>DB: Read mapping models
DB-->>Match: mappings/normalization
Match-->>DeviceView: Matched bay/type info
DeviceView->>Cache: Store merged inventory (cache key includes server_key/use_sysname/strip_domain)
DeviceView-->>User: Render module table with install actions
User->>DeviceView: Click Install Module/Branch
DeviceView->>Match: Resolve bay/type (mapping + normalization)
Match->>DB: Query/Create Module/ModuleBay
DB-->>DeviceView: Module created
DeviceView-->>User: Success + refresh
sequenceDiagram
participant User
participant ImportView
participant Validate
participant Utils as Utils (Mappings/Normalization)
participant DB as NetBox DB
participant LibreAPI as LibreNMS API
User->>ImportView: Start device import
ImportView->>Validate: validate_device_for_import(use_sysname, strip_domain, server_key)
Validate->>Utils: match_librenms_hardware_to_device_type
Utils->>DB: Query DeviceTypeMapping / ModuleTypeMapping
DB-->>Utils: mapping result
Utils-->>Validate: matched device type or fallback
Validate->>LibreAPI: optional inventory/VC checks
LibreAPI-->>Validate: inventory/VC
Validate->>ImportView: validation result (librenms_id_needs_migration?)
ImportView->>DB: set_librenms_device_id(obj, device_id, server_key)
DB-->>ImportView: Persisted
ImportView-->>User: Import result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
|
- _safe_disabled: clamp int conversion to strict 0/1 (not 2+) - bulk_import member_serials: normalize to str() before join - virtual_chassis: enforce position > 0, load pattern once before loop - device_operations: use set_librenms_device_id helper for new-device CF Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add TestVirtualChassisEdgeBranches covering create_virtual_chassis_with_members edge cases: zero/invalid position strings (lines 418-421), name conflict skip (lines 435-436), master name conflict, member serial matching master, duplicate serial skip, count warning - Add tests/test_vm_operations.py: 21 tests covering create_vm_from_librenms and bulk_import_vms including cancellation checkpoint path - Add tests/test_tables_modules.py: 54 tests covering all LibreNMSModuleTable render methods and table __init__/configure - Add 47 virtual_chassis tests to test_import_utils.py covering _clone, get, prefetch, detect, _load_pattern, _generate_name, update_suggested_names Coverage: virtual_chassis.py 100%, vm_operations.py 100%, tables/modules.py 100% 612 tests pass Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/test.yaml (1)
19-19:⚠️ Potential issue | 🟡 MinorPin
actions/upload-artifactto a specific SHA for consistency and security.Line 84 uses
actions/upload-artifact@v4without a SHA commit hash, while other actions in the workflow are pinned (checkout@v4, setup-python@v5). Pin it to a specific commit hash, e.g.,uses: actions/upload-artifact@<SHA> # v4.Regarding Python 3.14: This version is now fully released (as of late 2025) and available in
actions/setup-python. No additional configuration is needed; the workflow will install it successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yaml at line 19, The workflow uses an unpinned third-party action reference `uses: actions/upload-artifact@v4`; replace it with a commit-pinned reference like `uses: actions/upload-artifact@<SHA> # v4` to match the existing pinned actions (e.g., `checkout@v4`, `setup-python@v5`) and improve consistency/security—update the `uses: actions/upload-artifact@v4` entry (the line that configures the upload-artifact step) to include the specific commit SHA while keeping the v4 tag comment.docs/usage_tips/custom_field.md (1)
37-49:⚠️ Potential issue | 🟠 MajorManual field schema instructions are outdated for current
librenms_idformat.Line 47 still says
Type: Integer, and Lines 69-70 describe entering a scalar ID. Current behavior stores server-keyed values (e.g.,{"default": 123}), so following this section can break recreated-field compatibility in modern versions.🛠️ Suggested doc fix
- - **Type:** Integer + - **Type:** JSON - - Enter the LibreNMS device ID in the `librenms_id` field. + - Enter a server-keyed JSON value in the `librenms_id` field (for example: `{"default": 12345}`). + - If you are documenting truly legacy (<0.4.2) behavior, call out that older deployments used a scalar integer.Also applies to: 67-71
🤖 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 37 - 49, The docs incorrectly instruct creating librenms_id as an Integer and describing it as a scalar; update the Custom Field instructions for librenms_id to use a JSON/object-backed field (e.g., Type: JSON/Object) and update the Label/Description to explain the stored format is server-keyed (example: {"default": 123}), and replace any wording that says "enter scalar ID" with guidance to enter the server-keyed JSON structure so recreated fields remain compatible with current behavior (references: the librenms_id custom field instructions and any lines describing scalar ID input).netbox_librenms_plugin/import_utils/vm_operations.py (1)
22-27: 🧹 Nitpick | 🔵 TrivialDocument the new
server_keyparameter inArgs:.The signature was expanded at Line 17, but the docstring argument list hasn’t been updated.
📝 Minimal docstring update
Args: libre_device: Device data from LibreNMS validation: Validation result from validate_device_for_import with import_as_vm=True use_sysname: If True, prefer sysName; if False, use hostname role: Optional DeviceRole to assign to the VM + server_key: LibreNMS server identifier used for per-server librenms_id storage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/vm_operations.py` around lines 22 - 27, Update the docstring Args section in netbox_librenms_plugin/import_utils/vm_operations.py to include the new server_key parameter: add a concise description of server_key (type and purpose, e.g., optional SSH/private key or identifier used when creating the VM) alongside the existing libre_device, validation, use_sysname, and role entries so the docstring matches the function signature that now accepts server_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yaml:
- Around line 83-88: The workflow step named "Upload coverage report" currently
uses the floating tag actions/upload-artifact@v4; update that uses line to pin
the action to the specific commit SHA (replace `@v4` with @<commit-sha>) to match
how other steps are pinned (e.g., actions/checkout and actions/setup-python);
locate the step by the name "Upload coverage report" and change the uses field
to actions/upload-artifact@<exact-sha> so the workflow uses a fixed, auditable
release.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 412-420: Duplicate definition of _empty_return causes an F811
redefinition error; remove the redundant second definition so only a single
_empty_return(return_cache_status: bool) remains (preserve the existing
docstring and return logic) and ensure any calls from process_device_filters
still reference that single function name.
- Around line 375-399: The code currently retries lookup only with raw hostname
and sysName which misses cases where import naming transforms the name (e.g.,
use_sysname + strip_domain); update the fallback checks in bulk_import.py so
that if a resolved_name is available you use it for re-checking before raw
hostname/sysName: after the librenms_id block, attempt
Model.objects.filter(name__iexact=resolved_name).first() (and set match_type
accordingly, e.g., "resolved_name") before falling back to
Model.objects.filter(name__iexact=hostname).first() and
Model.objects.filter(name__iexact=sys_name).first(); ensure you reference the
existing symbols (librenms_id, hostname, sys_name, resolved_name,
find_by_librenms_id, Model.objects.filter(...).first()) and keep match_type
semantics consistent.
In `@netbox_librenms_plugin/models.py`:
- Around line 181-189: ModuleBayMapping.clean currently only compiles
librenms_name; also validate that netbox_bay_name replacement templates have
valid backreferences by compiling self.librenms_name into a pattern then
attempting a safe substitution (e.g. pattern.sub(self.netbox_bay_name, ""))
inside the try/except block and raise ValidationError({"netbox_bay_name":
f"Invalid replacement: {e}"}) on re.error/IndexError so templates like "\2" with
too few capture groups are rejected; update the clean method to perform both
checks (compile librenms_name and exercise netbox_bay_name against the compiled
pattern).
In `@netbox_librenms_plugin/tables/mappings.py`:
- Line 73: Extract the duplicated dict into a module-level constant (e.g.,
TABLE_CSS_ATTRS = {"class": "table table-hover table-headings table-striped"})
and replace each occurrence of attrs = {"class": "table table-hover
table-headings table-striped"} in the Meta classes (referenced in this file
around the Meta blocks for the mapping classes at the spots containing "attrs =
...") with attrs = TABLE_CSS_ATTRS so all Meta classes reuse the single
constant.
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 147-193: The final return in render_actions uses a redundant
nested format_html call; simplify by returning a single format_html around the
joined buttons (i.e., replace the double-wrapped format_html call that builds
from buttons with a single format_html("".join(str(b) for b in buttons)) or
equivalent), so the code returns a single safely formatted HTML string for the
buttons list.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping.html`:
- Around line 12-24: The detail template is missing the mapping's is_regex flag;
update modulebaymapping.html to add a header (e.g., "Regex?") and a
corresponding table cell that renders object.is_regex (use a human-friendly
display like Yes/No or a default "—" when undefined), for example by referencing
object.is_regex in the row so operators can see whether the mapping is
regex-based.
In `@netbox_librenms_plugin/tests/test_modules_view.py`:
- Around line 55-60: The mock queryset setup is fragile because it assigns
__class__ = list on mock_mapping.objects.filter.return_value; instead, replace
that workaround by constructing the return value as a MagicMock with a list spec
or by properly implementing iteration and length (e.g., use MagicMock(spec=list)
or set __iter__ and __len__ on mock_mapping.objects.filter.return_value) so
list() and iteration behave naturally; update the mock for
mock_mapping.objects.filter.return_value rather than mutating __class__ to
ensure future changes to how the code iterates the queryset won't break the
test.
In `@netbox_librenms_plugin/tests/test_permissions.py`:
- Around line 964-1072: Tests duplicate request/device/settings/transaction
mocks across _make_view and three test functions; extract shared fixtures in
tests/conftest.py and reuse them. Create fixtures that provide a request with
has_perm True (used by _make_view / RemoveServerMappingView), a mock_device and
mock_locked (with configurable custom_field_data, full_clean and save), a
patched settings fixture that sets PLUGINS_CONFIG, and mocked
messages/transaction contexts; update the tests
(test_validation_error_returns_error_message, test_configured_server_refused,
test_successful_removal_mutates_and_saves) to accept these fixtures and remove
the inline MagicMock/patch setup, and keep using Device and get_object_or_404
patches only where test-specific behavior is required.
In `@netbox_librenms_plugin/tests/test_sync_view_mismatch.py`:
- Around line 331-334: The helper _make_obj creating an ad-hoc MagicMock should
be removed and the tests should reuse the shared fixtures declared in
tests/conftest.py that produce model instances with proper custom_field_data;
replace calls to _make_obj(cf_librenms_id) with the appropriate fixture or
factory from conftest (the fixture that returns a model or object with a
custom_field_data dict) and set its
custom_field_data["librenms_id"]=cf_librenms_id in the test setup so model shape
and custom-field behavior remain consistent (update test function signatures to
accept the fixture or import the conftest factory where needed and delete the
_make_obj function).
In `@netbox_librenms_plugin/tests/test_tables_modules.py`:
- Around line 355-440: Tests repeat ad-hoc device mocks; replace them with the
shared device fixture from tests/conftest.py. Update each test function
(test_render_actions_...,
test_render_actions_can_install_renders_install_button,
test_render_actions_has_installable_children_renders_branch_button,
test_render_actions_both_buttons_rendered,
test_render_actions_installable_children_without_index_skips_branch,
test_render_actions_csrf_token_included_in_form) to accept the existing device
fixture name instead of creating MagicMock(), remove manual device.pk
assignments, and call self._make_table(device=device_fixture) (or the fixture
name used in conftest). Ensure imports/pytest signatures are adjusted so pytest
injects the fixture and run tests to verify no other changes are required.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 21-23: The current helper _librenms_id_q builds a combined OR Q
that can match both server-scoped and legacy librenms_id values, allowing
ambiguous matches; replace this with a deterministic two-step lookup: first
query using the server-scoped key
(custom_field_data__librenms_id__{server_key}=value) and if that returns no
result then query the legacy field (custom_field_data__librenms_id=value).
Update all call sites that relied on the combined Q (e.g., the
Device.objects.get() usage and the interface lookups that call .first() ) to use
this two-step lookup (or a new helper function like
get_by_librenms_id(server_key, value, Model)) so server-scoped matches are
always preferred and legacy is only used as a fallback, preserving existing
exception handling for MultipleObjectsReturned.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 107-112: The map interfaces_by_librenms_id uses raw lib_id values
that can be int or str, causing mismatches when later looked up by API port_id;
normalize keys consistently by coercing lib_id to a stable type (e.g., str) when
populating the map in the loop that calls get_librenms_device_id(interface,
server_key) (use interfaces_by_librenms_id[str(lib_id)] = interface) and ensure
any subsequent lookups use the same normalization (e.g., str(port_id)) so
lookups against interfaces_by_librenms_id succeed reliably.
In `@netbox_librenms_plugin/views/base/librenms_sync_view.py`:
- Around line 341-387: Remove the duplicate _build_all_server_mappings
definition (the later one that directly accesses django_settings.PLUGINS_CONFIG
and lacks the legacy "default" fallback); keep the original implementation (the
earlier _build_all_server_mappings) as the single source of truth, restore its
legacy fallback that uses the root-level librenms_url when server_key ==
"default" and no matching servers entry, and ensure the PLUGINS_CONFIG is
accessed with the safe getattr pattern used in the first implementation so only
one correct _build_all_server_mappings remains.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 1002-1004: The ModuleBay queryset restricts results with
module_id__isnull=True which excludes bays belonging to installed child modules
and breaks nested parent resolution; update the queries that use
ModuleBay.objects.filter(device=device,
module_id__isnull=True).select_related("installed_module") to remove the
module_id__isnull filter (i.e., filter only by device=device and keep
select_related("installed_module")) so module-scoped bays are included, and
apply the same change to the other identical query used later (the block around
the second ModuleBay.objects.filter call).
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1231-1253: The migrate_librenms_id action must guard against
another device already owning the same LibreNMS ID for this server; inside the
existing transaction.atomic() after re-checking the locked_device (and before
calling migrate_legacy_librenms_id), query for any Device with the same
custom_field_data librenms_id and librenms_server matching
self.librenms_api.server_key that is not the current device (use
Device.objects.filter(...).exclude(pk=locked_device.pk).exists() like the
link/update/update_serial pattern) and return an HTTP conflict response if such
a device exists; keep using the locked_device and then call
migrate_legacy_librenms_id(locked_device, self.librenms_api.server_key) and
_save_device only if no conflict is found.
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 130-137: The except block currently treats all runtime exceptions
as data issues by appending to results["invalid"]; change it to record
operational errors separately: in the loop over selected_interfaces, keep the
logger.exception("Failed to sync cable for interface %s",
interface.get("interface", "")) but append the interface (and optionally the
exception message) to a new results["error"] (or "failed") bucket instead of
results["invalid"], so process_single_interface() results still populate
status-based buckets and true data-validation failures remain under
results["invalid"]; update any callers/consumers that expect the results shape
to handle the new "error"/"failed" key.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 244-246: The sync handler currently skips MAC processing when the
guard uses is_device_interface, which prevents VM interface MACs from being
synced while the UI/table (LibreNMSVMInterfaceTable) and comparison logic
(_compare_mac_addresses calling netbox_interface.mac_addresses.all()) expect VM
MACs to exist; update the condition around the call to
self.handle_mac_address(interface, ifPhysAddress) so VMInterface instances are
allowed (e.g., extend the predicate to include VMInterface or remove the
device-only restriction), ensuring the MAC sync path in the function that calls
handle_mac_address will run for VM interfaces as well.
In `@tests/e2e/test_module_install.py`:
- Around line 97-121: Add an explicit opt-in guard to the e2e fixtures so they
don't run unconditionally: in the browser and page fixtures (symbols: browser,
page) check an environment variable like RUN_E2E (and required prerequisites
such as NETBOX_URL, NETBOX_USER, NETBOX_PASS) at the start and call
pytest.skip(...) if not present/valid; only start sync_playwright()/launch the
browser and perform the login when RUN_E2E is truthy and the NetBox env vars are
set to avoid hard failures in non-devcontainer environments.
- Around line 133-140: Replace the fixed time.sleep(2) and time.sleep(8) around
locating and clicking the Refresh Modules button with event-driven Playwright
waits: use page.wait_for_selector('button:has-text("Refresh Modules")') to wait
for the button to become available, call btn = page.query_selector(...) and
btn.click(), then wait for a concrete post-click condition (e.g.,
page.wait_for_selector for a success toast, wait_for_selector/spinner
disappearance for the modules list, or page.wait_for_response matching the
refresh API) instead of time.sleep; update the test to remove both time.sleep
calls and use these waits around the selectors and the click to make the test
deterministic.
---
Outside diff comments:
In @.github/workflows/test.yaml:
- Line 19: The workflow uses an unpinned third-party action reference `uses:
actions/upload-artifact@v4`; replace it with a commit-pinned reference like
`uses: actions/upload-artifact@<SHA> # v4` to match the existing pinned actions
(e.g., `checkout@v4`, `setup-python@v5`) and improve consistency/security—update
the `uses: actions/upload-artifact@v4` entry (the line that configures the
upload-artifact step) to include the specific commit SHA while keeping the v4
tag comment.
In `@docs/usage_tips/custom_field.md`:
- Around line 37-49: The docs incorrectly instruct creating librenms_id as an
Integer and describing it as a scalar; update the Custom Field instructions for
librenms_id to use a JSON/object-backed field (e.g., Type: JSON/Object) and
update the Label/Description to explain the stored format is server-keyed
(example: {"default": 123}), and replace any wording that says "enter scalar ID"
with guidance to enter the server-keyed JSON structure so recreated fields
remain compatible with current behavior (references: the librenms_id custom
field instructions and any lines describing scalar ID input).
In `@netbox_librenms_plugin/import_utils/vm_operations.py`:
- Around line 22-27: Update the docstring Args section in
netbox_librenms_plugin/import_utils/vm_operations.py to include the new
server_key parameter: add a concise description of server_key (type and purpose,
e.g., optional SSH/private key or identifier used when creating the VM)
alongside the existing libre_device, validation, use_sysname, and role entries
so the docstring matches the function signature that now accepts server_key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: da28f17f-b8fb-4404-b21b-3b2b31796530
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (88)
.devcontainer/README.md.devcontainer/scripts/diagnose.sh.devcontainer/scripts/load-aliases.sh.devcontainer/scripts/setup.sh.devcontainer/scripts/start-netbox.sh.devcontainer/scripts/welcome.sh.github/pull_request_template.md.github/workflows/lint-format.yaml.github/workflows/test.yamlREADME.mdcontrib/README.mdcontrib/device_type_mappings.yamlcontrib/interface_name_rules.yamlcontrib/interface_type_mappings.yamlcontrib/module_bay_mappings.yamlcontrib/module_type_mappings.yamlcontrib/normalization_rules.yamldocs/usage_tips/custom_field.mddocs/usage_tips/permissions.mdnetbox_librenms_plugin/__init__.pynetbox_librenms_plugin/api/serializers.pynetbox_librenms_plugin/api/urls.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/filters.pynetbox_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/migrations/0009_add_devicetypemapping.pynetbox_librenms_plugin/migrations/0010_add_moduletypemapping.pynetbox_librenms_plugin/migrations/0011_modulebaymapping.pynetbox_librenms_plugin/migrations/0012_add_is_regex_to_modulebaymapping.pynetbox_librenms_plugin/migrations/0013_normalizationrule.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/mappings.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_init.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.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/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/object_sync/__init__.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.pytests/e2e/__init__.pytests/e2e/conftest.pytests/e2e/test_module_install.py
Comment on lines
+83
to
+88
| - name: Upload coverage report | ||
| uses: actions/upload-artifact@v4 | ||
| if: matrix.python-version == '3.12' | ||
| with: | ||
| name: coverage-report | ||
| path: netbox-librenms-plugin/coverage_html/ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/test.yaml | head -100Repository: marcinpsk/netbox-librenms-plugin
Length of output: 3101
🌐 Web query:
actions/upload-artifact v4 latest commit SHA github
💡 Result:
The actions/upload-artifact@v4 tag currently points to commit:
ea165f8d65b6e75b540449e92b4886f43607fa02 [1][2]
Pin actions/upload-artifact to a SHA for consistency.
Other actions in this workflow (actions/checkout, actions/setup-python) are pinned to specific commit SHAs for security. The upload-artifact action should follow the same pattern.
🔒 Proposed fix to pin the action SHA
- name: Upload coverage report
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4
if: matrix.python-version == '3.12'
with:
name: coverage-report
path: netbox-librenms-plugin/coverage_html/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yaml around lines 83 - 88, The workflow step named
"Upload coverage report" currently uses the floating tag
actions/upload-artifact@v4; update that uses line to pin the action to the
specific commit SHA (replace `@v4` with @<commit-sha>) to match how other steps
are pinned (e.g., actions/checkout and actions/setup-python); locate the step by
the name "Upload coverage report" and change the uses field to
actions/upload-artifact@<exact-sha> so the workflow uses a fixed, auditable
release.
| "description", | ||
| "actions", | ||
| ) | ||
| attrs = {"class": "table table-hover table-headings table-striped"} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract shared table CSS attrs into a module-level constant.
The repeated attrs = {"class": "table table-hover table-headings table-striped"} across multiple Meta classes is a small duplication hotspot.
♻️ Proposed cleanup
+TABLE_ATTRS = {"class": "table table-hover table-headings table-striped"}
...
- attrs = {"class": "table table-hover table-headings table-striped"}
+ attrs = TABLE_ATTRSAlso applies to: 102-102, 137-137, 173-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tables/mappings.py` at line 73, Extract the duplicated
dict into a module-level constant (e.g., TABLE_CSS_ATTRS = {"class": "table
table-hover table-headings table-striped"}) and replace each occurrence of attrs
= {"class": "table table-hover table-headings table-striped"} in the Meta
classes (referenced in this file around the Meta blocks for the mapping classes
at the spots containing "attrs = ...") with attrs = TABLE_CSS_ATTRS so all Meta
classes reuse the single constant.
Comment on lines
+130
to
+137
| for interface in selected_interfaces: | ||
| try: | ||
| with transaction.atomic(): | ||
| result = self.process_single_interface(interface, cached_links) | ||
| results[result["status"]].append(result.get("interface", "")) | ||
| except Exception: | ||
| logger.exception("Failed to sync cable for interface %s", interface.get("interface", "")) | ||
| results["invalid"].append(interface.get("interface", "")) |
There was a problem hiding this comment.
Unexpected runtime errors are currently mislabeled as “invalid link data.”
Line 137 appends any exception path into results["invalid"], which later produces the “No LibreNMS link data found…” message. That masks operational/system failures as data-quality failures.
🔧 Proposed fix
def process_interface_sync(self, selected_interfaces, cached_links):
@@
- results = {"valid": [], "invalid": [], "duplicate": [], "missing_remote": []}
+ results = {"valid": [], "invalid": [], "duplicate": [], "missing_remote": [], "error": []}
@@
except Exception:
logger.exception("Failed to sync cable for interface %s", interface.get("interface", ""))
- results["invalid"].append(interface.get("interface", ""))
+ results["error"].append(interface.get("interface", ""))Also add a dedicated message branch:
def display_sync_results(self, request, results):
@@
+ if results["error"]:
+ messages.error(
+ request,
+ f"Cable sync failed due to internal errors for: {', '.join(results['error'])}",
+ )🤖 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 130 - 137, The
except block currently treats all runtime exceptions as data issues by appending
to results["invalid"]; change it to record operational errors separately: in the
loop over selected_interfaces, keep the logger.exception("Failed to sync cable
for interface %s", interface.get("interface", "")) but append the interface (and
optionally the exception message) to a new results["error"] (or "failed") bucket
instead of results["invalid"], so process_single_interface() results still
populate status-based buckets and true data-validation failures remain under
results["invalid"]; update any callers/consumers that expect the results shape
to handle the new "error"/"failed" key.
Comment on lines
+97
to
+121
| @pytest.fixture(scope="module") | ||
| def browser(): | ||
| """Launch browser for the test module.""" | ||
| from playwright.sync_api import sync_playwright | ||
|
|
||
| pw = sync_playwright().start() | ||
| b = pw.chromium.launch(headless=True) | ||
| yield b | ||
| b.close() | ||
| pw.stop() | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def page(browser): | ||
| """Create a new page and log in to NetBox.""" | ||
| ctx = browser.new_context(ignore_https_errors=True) | ||
| pg = ctx.new_page() | ||
|
|
||
| pg.goto(f"{NETBOX_URL}/login/", timeout=10000) | ||
| pg.fill("#id_username", NETBOX_USER) | ||
| pg.fill("#id_password", NETBOX_PASS) | ||
| pg.click("button[type=submit]") | ||
| pg.wait_for_load_state("networkidle") | ||
| yield pg | ||
| ctx.close() |
There was a problem hiding this comment.
Gate E2E execution behind an explicit environment check
The fixtures run unconditionally and immediately attempt browser login, which will hard-fail in environments without the devcontainer/NetBox stack. Add an opt-in guard (for example, RUN_E2E=1) and skip early when prerequisites are missing.
Proposed guard
+@pytest.fixture(scope="module", autouse=True)
+def require_e2e_environment():
+ if os.getenv("RUN_E2E") != "1":
+ pytest.skip("Set RUN_E2E=1 to run e2e tests.")
+ _get_container()
+
`@pytest.fixture`(scope="module")
-def browser():
+def browser(require_e2e_environment):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test_module_install.py` around lines 97 - 121, Add an explicit
opt-in guard to the e2e fixtures so they don't run unconditionally: in the
browser and page fixtures (symbols: browser, page) check an environment variable
like RUN_E2E (and required prerequisites such as NETBOX_URL, NETBOX_USER,
NETBOX_PASS) at the start and call pytest.skip(...) if not present/valid; only
start sync_playwright()/launch the browser and perform the login when RUN_E2E is
truthy and the NetBox env vars are set to avoid hard failures in
non-devcontainer environments.
Comment on lines
+133
to
+140
| time.sleep(2) | ||
|
|
||
| # Click Refresh Modules | ||
| btn = page.query_selector('button:has-text("Refresh Modules")') | ||
| assert btn is not None, "Refresh Modules button not found" | ||
| btn.click() | ||
| time.sleep(8) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace fixed sleeps with event-driven Playwright waits
Using fixed delays here (and throughout this file) makes the suite flaky under variable load. Wait for concrete UI/network conditions instead.
Example pattern
- time.sleep(8)
+ page.locator("#modules table tr").first.wait_for(state="visible", timeout=20000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test_module_install.py` around lines 133 - 140, Replace the fixed
time.sleep(2) and time.sleep(8) around locating and clicking the Refresh Modules
button with event-driven Playwright waits: use
page.wait_for_selector('button:has-text("Refresh Modules")') to wait for the
button to become available, call btn = page.query_selector(...) and btn.click(),
then wait for a concrete post-click condition (e.g., page.wait_for_selector for
a success toast, wait_for_selector/spinner disappearance for the modules list,
or page.wait_for_response matching the refresh API) instead of time.sleep;
update the test to remove both time.sleep calls and use these waits around the
selectors and the click to make the test deterministic.
…odules tab port names
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
netbox_librenms_plugin/views/base/modules_view.py (1)
1035-1037:⚠️ Potential issue | 🟠 MajorInclude module-scoped bays in parent-module resolution.
Line 1035andLine 1051filter withmodule_id__isnull=True, which excludes module-owned bays. Nested descendants under already-installed child modules can’t resolve their actual parent module and may be skipped/mis-scoped.Proposed fix
- device_bays = ModuleBay.objects.filter(device=device, module_id__isnull=True).select_related( + device_bays = ModuleBay.objects.filter(device=device).select_related( "installed_module" ) @@ - bay = ( - ModuleBay.objects.filter(device=device, name=mapping.netbox_bay_name, module_id__isnull=True) - .select_related("installed_module") - .first() - ) + bay = ( + ModuleBay.objects.filter(device=device, name=mapping.netbox_bay_name) + .select_related("installed_module") + .first() + )Also applies to: 1051-1053
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 1035 - 1037, The ModuleBay queryset uses module_id__isnull=True which incorrectly excludes module-owned bays and prevents resolving parent modules for nested child-module descendants; update the queries that assign device_bays (and the similar queryset around the parent-module resolution) to remove the module_id__isnull=True filter (or otherwise include module-scoped bays) so ModuleBay.objects.filter(device=device).select_related("installed_module") returns both device- and module-owned bays, ensuring nested descendant parent-module resolution works correctly.
🤖 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/templates/netbox_librenms_plugin/librenms_sync_base.html`:
- Around line 75-86: The form that POSTS to remove_server_mapping currently
always uses object.pk (action uses remove_server_mapping with pk=object.pk) but
the base template is rendered for VMs too, causing wrong Device resolution on PK
collisions; guard the form so it only renders/ submits when the page object is a
Device (e.g. check object._meta.model_name == 'device' or equivalent template
predicate) and keep the existing fields (mapping.server_key, input
name="server_key") and button unchanged; update the surrounding template to only
include the form block when the object is a Device so remove_server_mapping is
never invoked from VM pages.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 402-414: The recursive traversal in _collect_descendants (and the
similar recursion at lines ~922-930) lacks cycle protection for
entPhysicalContainedIn/entPhysicalIndex chains; add a visited set (e.g.,
visited_indexes) passed through the recursion to record entPhysicalIndex values
you've already expanded, skip recursing into a child if its entPhysicalIndex is
in visited_indexes, and add the current child index to the set before recursing
(ensure you clone or remove on return if using a shared set) so malformed or
cyclic inventory_data cannot cause infinite recursion while preserving existing
depth/results behavior.
- Around line 459-477: Extract the duplicated _get_module_types logic into a
single shared helper (e.g., a mixin class or module-level function) and have
both callers use that helper instead of their own implementations; specifically
move the existing logic that imports dcim.models.ModuleType and
netbox_librenms_plugin.models.ModuleTypeMapping and builds the result dict into
a single function (e.g., get_module_types() or
ModuleTypeLookupMixin._get_module_types) and replace the duplicate
implementations with a call to that function so both classes use the same code
path for result[mapping.librenms_model] = mapping.netbox_module_type and the
part_number/model fallback behavior.
- Around line 774-1105: InstallModuleView and InstallBranchView are sync-action
handlers placed in views/base which breaks the 3-layer architecture; move these
classes (including their helper methods _collect_branch, _collect_children,
_get_module_types, _install_single, _find_parent_module_id, _match_bay and any
ModuleBayMapping/module-related imports) into a new module under views/sync/ (or
the existing sync layer), update their base mixins to use the shared mixins from
views/mixins.py, adjust imports/URL references to point to the new location, and
remove the original definitions from views/base to keep only base view
primitives there; ensure any references (tests, URL patterns, other views) are
updated and run tests to validate behavior.
---
Duplicate comments:
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 1035-1037: The ModuleBay queryset uses module_id__isnull=True
which incorrectly excludes module-owned bays and prevents resolving parent
modules for nested child-module descendants; update the queries that assign
device_bays (and the similar queryset around the parent-module resolution) to
remove the module_id__isnull=True filter (or otherwise include module-scoped
bays) so
ModuleBay.objects.filter(device=device).select_related("installed_module")
returns both device- and module-owned bays, ensuring nested descendant
parent-module resolution works correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 067cee04-cc41-402c-9128-37d4aeb217db
📒 Files selected for processing (2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/views/base/modules_view.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). (2)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.12)
🧰 Additional context used
📓 Path-based instructions (6)
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/views/base/modules_view.py
netbox_librenms_plugin/views/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/views/**/*.py: Views must follow a three-layer structure: Base views inviews/base/, Object sync views inviews/object_sync/, and Sync action views inviews/sync/with shared mixins fromviews/mixins.py
New views must extend the closest base class and compose mixins fromviews/mixins.py(LibreNMSPermissionMixin,NetBoxObjectPermissionMixin,LibreNMSAPIMixin,CacheMixin,VlanAssignmentMixin)
All views must inheritLibreNMSPermissionMixinfromviews/mixins.pywithpermission_required = PERM_VIEW_PLUGIN
Declarerequired_object_permissionsas a dict mapping HTTP methods to[(action, Model)]tuples for NetBox model operations; some views may set this dynamically per-request
Use_get_safe_redirect_url(request)to validate referrer URLs in permission checks to prevent open-redirect attacks
Files:
netbox_librenms_plugin/views/base/modules_view.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/modules_view.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow frontend conventions for templates and static files defined in
.github/instructions/frontend.instructions.md
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer for table row updates. Table row updates must return<tr hx-swap-oob="true">. AvoidouterHTMLswaps; use OOB (Out-of-Band) or targetedinnerHTMLswaps to keep table layout intact.
Styling assumes Tabler defaults. Removingtable-responsivewrappers was deliberate to prevent dropdown clipping—do not re-add them.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
netbox_librenms_plugin/{templates,static}/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/{templates,static}/**/*.{html,js}: All HTMX requests andfetch()calls must include a CSRF token. The standard pattern isdocument.querySelector('[name=csrfmiddlewaretoken]').value(from a hidden form input). The import JS also usesgetCookie('csrftoken')as a fallback — prefer the hidden input approach for consistency.
Modals use Tabler (Bootstrap-like) but withoutbootstrap.Modalhelpers. Buttons target thehtmx-modal-contentelement and JavaScript inlibrenms_import.htmltoggles the wrapper. Do not reintroducedata-bs-toggleor duplicate modal IDs.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
netbox_librenms_plugin/{templates,tables}/**/*.{html,py}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Templates live in
templates/netbox_librenms_plugin/; reuse/includes underinc/. Sync pages extendlibrenms_sync_base.html. Tables emit HTMX-enabled columns and buttons (tables/*.py), so prefer updating the table renderer in Python rather than templates when changing row actions.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
🧠 Learnings (22)
📚 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/views/**/*.py : Views must follow a three-layer structure: Base views in `views/base/`, Object sync views in `views/object_sync/`, and Sync action views in `views/sync/` with shared mixins from `views/mixins.py`
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 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/views/**/*.py : New views must extend the closest base class and compose mixins from `views/mixins.py` (`LibreNMSPermissionMixin`, `NetBoxObjectPermissionMixin`, `LibreNMSAPIMixin`, `CacheMixin`, `VlanAssignmentMixin`)
Applied to files:
netbox_librenms_plugin/views/base/modules_view.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 : Hook into NetBox Django 5 plugin APIs via `navigation.py`, `urls.py`, and `api/` modules under `netbox_librenms_plugin/`; respect NetBox plugin conventions
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 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/views/sync/**/*.py : Follow sync conventions defined in `.github/instructions/sync.instructions.md` for sync views, base views, tables, and sync JavaScript
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : Base view classes (`BaseLibreNMSSyncView`, `BaseInterfaceTableView`, `BaseCableTableView`, `BaseIPAddressTableView`, `BaseVLANTableView`) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results with `CacheMixin` keys like `librenms_{data_type}_{model_name}_{pk}`, compare against NetBox objects, and render a django-tables2 table in a partial template.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/{templates,tables}/**/*.{html,py} : Templates live in `templates/netbox_librenms_plugin/`; reuse/includes under `inc/`. Sync pages extend `librenms_sync_base.html`. Tables emit HTMX-enabled columns and buttons (`tables/*.py`), so prefer updating the table renderer in Python rather than templates when changing row actions.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: All base view classes must follow the three-layer architecture: Base views define abstract data pipelines via `get_*_context()` methods, object sync views wire base views to NetBox models using `register_model_view()`, and sync action views handle POST requests with permissions checks and transactional updates.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.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 : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/sync/**/*.py : Sync action views must follow the pattern: check permissions with `LibreNMSPermissionMixin` and `NetBoxObjectPermissionMixin`, read selected items from `request.POST.getlist('select')`, load cached data using `CacheMixin.get_cache_key()`, apply changes inside `transaction.atomic()`, and redirect to the sync tab with `?tab=<resource>`.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 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/views/**/*.py : All views must inherit `LibreNMSPermissionMixin` from `views/mixins.py` with `permission_required = PERM_VIEW_PLUGIN`
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.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 `get_virtual_chassis_member()` for port-to-member mapping and `get_librenms_sync_device()` for VC priority-based device selection in virtual chassis operations
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : `VlanAssignmentMixin` must resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/htmx/*.html : HTMX fragments live in `templates/netbox_librenms_plugin/htmx/` and include: `device_import_row.html` (individual import row updates), `device_validation_details.html` (expandable validation details), `device_vc_details.html` (virtual chassis member details), `bulk_import_confirm.html` (import confirmation modal content). Keep server responses and HTMX targets in sync when modifying these fragments.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/_*_sync{,_content}.html : Each sync resource has two templates following a naming convention: `_<resource>_sync.html` (the tab wrapper, loaded once when the tab is selected) and `_<resource>_sync_content.html` (the HTMX-swappable inner fragment, refreshed on data changes without a full page reload). Current resources: `_interface_sync`, `_cable_sync`, `_ipaddress_sync`, `_vlan_sync`. When adding a new sync resource, create both the wrapper and content templates following this pattern.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/{templates,static}/**/*.{html,js} : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers. Buttons target the `htmx-modal-content` element and JavaScript in `librenms_import.html` toggles the wrapper. Do not reintroduce `data-bs-toggle` or duplicate modal IDs.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 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: Keep REST endpoint server responses and HTMX targets in sync; implement REST endpoints in `views/imports/actions.py` and HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 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: Sync pipelines must follow the pattern: fetch LibreNMS data via `librenms_api.py`, cache with `CacheMixin`, build comparison tables via `tables/*.py`, and render HTMX fragments from `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 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 : Reuse `librenms_api.py` client for all LibreNMS communication instead of making direct `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and caching
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Styling assumes Tabler defaults. Removing `table-responsive` wrappers was deliberate to prevent dropdown clipping—do not re-add them.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript URL and tab state management must implement `initializeTabs()`, `getDeviceIdFromUrl()`, and `setInterfaceNameFieldFromURL()` to maintain browser state and URL synchronization.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/inc/paginator.html : `inc/paginator.html` is a custom paginator that preserves tab state and `interface_name_field` in pagination URLs. Used across all sync tables.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
🔇 Additional comments (6)
netbox_librenms_plugin/views/base/modules_view.py (1)
54-85: Good use of plugin API + cache pipeline here.
Line 58correctly usesLibreNMSAPI.get_librenms_id(), andLines 81-85cache merged inventory viaCacheMixinkeying/timeout as expected.As per coding guidelines: "Use
LibreNMSAPI.get_librenms_id()instead of directly accessing thelibrenms_idcustom field..." and base views should "fetch data from LibreNMS API, cache results withCacheMixinkeys...".netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html (5)
32-73: Multi-server connection card rendering looks solidThe per-server status rendering (active/configured/unconfigured) and device-link fallback behavior are clear and robust.
106-112: Active server block now correctly surfaces device deep-linkShowing
librenms_device_urlwith ID in the legacy fallback card is a useful UX improvement.
321-332:sysNamesync guard is correctLine 321/Line 331 correctly avoid using placeholder
"-"and suppress unnecessary sync actions when the name already matches.
607-614: Modules tab gating is correctly conditionalConditionally rendering the Modules tab only when
module_syncis present matches the existing tab pattern.
644-649:⚠️ Potential issue | 🟡 MinorMove module sync wrapper to main directory for pattern consistency
The paired
_module_sync_content.htmltemplate exists and HTMX targeting is correctly configured (hx-target="#module-sync-content"). However,_module_sync.htmlis located in theinc/subdirectory while all other sync resource wrappers (_interface_sync.html,_cable_sync.html,_vlan_sync.html,_ipaddress_sync.html) are in the main templates directory. Move_module_sync.htmlfrominc/tonetbox_librenms_plugin/templates/netbox_librenms_plugin/to maintain consistent template structure across all sync resources.⛔ Skipped due to learnings
Learnt from: CR Repo: marcinpsk/netbox-librenms-plugin PR: 0 File: .github/instructions/frontend.instructions.md:0-0 Timestamp: 2026-03-03T13:24:18.134Z Learning: Applies to netbox_librenms_plugin/templates/**/_*_sync{,_content}.html : Each sync resource has two templates following a naming convention: `_<resource>_sync.html` (the tab wrapper, loaded once when the tab is selected) and `_<resource>_sync_content.html` (the HTMX-swappable inner fragment, refreshed on data changes without a full page reload). Current resources: `_interface_sync`, `_cable_sync`, `_ipaddress_sync`, `_vlan_sync`. When adding a new sync resource, create both the wrapper and content templates following this pattern.Learnt from: CR Repo: marcinpsk/netbox-librenms-plugin PR: 0 File: .github/instructions/frontend.instructions.md:0-0 Timestamp: 2026-03-03T13:24:18.134Z Learning: Applies to netbox_librenms_plugin/{templates,tables}/**/*.{html,py} : Templates live in `templates/netbox_librenms_plugin/`; reuse/includes under `inc/`. Sync pages extend `librenms_sync_base.html`. Tables emit HTMX-enabled columns and buttons (`tables/*.py`), so prefer updating the table renderer in Python rather than templates when changing row actions.Learnt from: CR Repo: marcinpsk/netbox-librenms-plugin PR: 0 File: .github/instructions/frontend.instructions.md:0-0 Timestamp: 2026-03-03T13:24:18.134Z Learning: Applies to netbox_librenms_plugin/templates/**/htmx/*.html : HTMX fragments live in `templates/netbox_librenms_plugin/htmx/` and include: `device_import_row.html` (individual import row updates), `device_validation_details.html` (expandable validation details), `device_vc_details.html` (virtual chassis member details), `bulk_import_confirm.html` (import confirmation modal content). Keep server responses and HTMX targets in sync when modifying these fragments.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/views/sync/**/*.py : Follow sync conventions defined in `.github/instructions/sync.instructions.md` for sync views, base views, tables, and sync JavaScriptLearnt 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/templates/**/*.html : Follow frontend conventions for templates and static files defined in `.github/instructions/frontend.instructions.md`Learnt from: CR Repo: marcinpsk/netbox-librenms-plugin PR: 0 File: .github/instructions/frontend.instructions.md:0-0 Timestamp: 2026-03-03T13:24:18.134Z Learning: Applies to netbox_librenms_plugin/{templates,static}/**/*.{html,js} : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers. Buttons target the `htmx-modal-content` element and JavaScript in `librenms_import.html` toggles the wrapper. Do not reintroduce `data-bs-toggle` or duplicate modal IDs.Learnt from: CR Repo: marcinpsk/netbox-librenms-plugin PR: 0 File: .github/instructions/frontend.instructions.md:0-0 Timestamp: 2026-03-03T13:24:18.134Z Learning: Applies to netbox_librenms_plugin/templates/**/*.html : HTMX 2.x is the primary async layer for table row updates. Table row updates must return `<tr hx-swap-oob="true">`. Avoid `outerHTML` swaps; use OOB (Out-of-Band) or targeted `innerHTML` swaps to keep table layout intact.Learnt from: CR Repo: marcinpsk/netbox-librenms-plugin PR: 0 File: .github/instructions/sync.instructions.md:0-0 Timestamp: 2026-03-03T13:24:38.082Z Learning: Applies to **/views/sync/**/*.py : Sync action views must follow the pattern: check permissions with `LibreNMSPermissionMixin` and `NetBoxObjectPermissionMixin`, read selected items from `request.POST.getlist('select')`, load cached data using `CacheMixin.get_cache_key()`, apply changes inside `transaction.atomic()`, and redirect to the sync tab with `?tab=<resource>`.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: Keep REST endpoint server responses and HTMX targets in sync; implement REST endpoints in `views/imports/actions.py` and HTMX fragments in `templates/netbox_librenms_plugin/htmx/`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/views/**/*.py : Views must follow a three-layer structure: Base views in `views/base/`, Object sync views in `views/object_sync/`, and Sync action views in `views/sync/` with shared mixins from `views/mixins.py`Learnt from: CR Repo: marcinpsk/netbox-librenms-plugin PR: 0 File: .github/instructions/sync.instructions.md:0-0 Timestamp: 2026-03-03T13:24:38.082Z Learning: Applies to **/librenms_sync.js : JavaScript URL and tab state management must implement `initializeTabs()`, `getDeviceIdFromUrl()`, and `setInterfaceNameFieldFromURL()` to maintain browser state and URL synchronization.
- SyncInterfacesView: add LibreNMSAPIMixin to MRO (fixes AttributeError on librenms_api.server_key) - bulk_import: remove duplicate _empty_return function definition (F811) - librenms_sync_view: remove duplicate _build_all_server_mappings that shadowed the one with legacy 'default' key fallback and safe PLUGINS_CONFIG access - virtual_chassis: prevent VC position collision when multiple members report the same SNMP position (track used_positions set); extend _generate_vc_member_name exception handling to also catch ValueError and IndexError - librenms_sync_base.html: guard remove_server_mapping form with device-only check to prevent wrong URL resolution on VM sync pages - modules_view: remove module_id__isnull=True filter in _find_parent_module_id so nested (module-scoped) bays are included in parent resolution - ModuleBayMapping.clean: validate netbox_bay_name replacement template backreferences when is_regex=True - modulebaymapping.html: display is_regex field in detail template - ip_addresses_view: normalize interfaces_by_librenms_id keys to str to prevent type mismatch on port_id lookups - vm_operations: add server_key to create_vm_from_librenms docstring
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
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/import_utils/vm_operations.py (1)
16-68: 🧹 Nitpick | 🔵 TrivialConsider using
set_librenms_device_idhelper for consistency.Line 64 directly constructs the
librenms_iddict inline. While this works for new VMs, the codebase has a dedicated helperset_librenms_device_idinutils.py(context snippet 2) that provides:
- Validation that
device_idis a valid integer (with warning on failure)- Migration logic for legacy bare-integer values
- Consistent error handling
For new object creation, this is less critical since there's no legacy data to migrate, but using the helper ensures consistency if the storage format evolves.
♻️ Optional refactor for consistency
+ from ..utils import set_librenms_device_id + # Create the VM with librenms_id custom field vm = VirtualMachine.objects.create( name=vm_name, cluster=cluster, role=role, # Optional VM role platform=platform, comments=f"Imported from LibreNMS by netbox-librenms-plugin on {import_time}", - custom_field_data={"librenms_id": {server_key: int(libre_device["device_id"])}}, + custom_field_data={}, ) + set_librenms_device_id(vm, libre_device["device_id"], server_key) + vm.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/vm_operations.py` around lines 16 - 68, The create_vm_from_librenms function currently inlines custom_field_data for librenms_id; instead, create the VirtualMachine as before but omit constructing librenms_id inline and then call the existing helper set_librenms_device_id(vm, libre_device["device_id"], server_key) from utils.py to set the custom field (the helper will validate/convert/migrate and save appropriately); reference the function names create_vm_from_librenms and set_librenms_device_id so you can locate and update the code accordingly.
♻️ Duplicate comments (6)
netbox_librenms_plugin/views/sync/interfaces.py (1)
247-249:⚠️ Potential issue | 🟡 MinorRe-check device-only MAC sync guard at Line 247.
This guard still excludes VM interfaces from MAC updates, which can drift from VM interface comparison/display behavior if those paths still consume MAC data.
Suggested adjustment (if VM MAC usage is still expected)
- if "mac_address" not in exclude_columns and is_device_interface: + if "mac_address" not in exclude_columns and hasattr(interface, "mac_addresses"): ifPhysAddress = librenms_interface.get("ifPhysAddress") self.handle_mac_address(interface, ifPhysAddress)#!/bin/bash # Verify whether VM interface table/compare paths still depend on MAC fields. rg -nP --type=py -C3 'class\s+LibreNMSVMInterfaceTable|_compare_mac_addresses|netbox_interface\.mac_addresses|primary_mac_address|mac_address' netbox_librenms_plugin/tables/interfaces.py rg -nP --type=py -C3 'VMInterface|mac_addresses|primary_mac_address|mac_address' netbox_librenms_plugin/views netbox_librenms_plugin/testsExpected verification result: if VM table/comparison paths use MAC fields, keep MAC sync enabled for VM interfaces too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 247 - 249, The current guard prevents MAC updates for non-device interfaces by checking is_device_interface before calling handle_mac_address; if VM interface table/compare paths still rely on MAC fields, remove the is_device_interface condition so MACs are handled for all interfaces unless explicitly excluded via exclude_columns. Concretely, in the block containing exclude_columns, is_device_interface, librenms_interface.get("ifPhysAddress") and the call to self.handle_mac_address(interface, ifPhysAddress), change the if to only check "mac_address" not in exclude_columns (i.e., drop the is_device_interface check) so librenms_interface ifPhysAddress is processed for VM interfaces as well.netbox_librenms_plugin/import_utils/bulk_import.py (1)
391-399:⚠️ Potential issue | 🟠 MajorUse
resolved_namewhen re-checking newly imported objects.The fallback lookups at lines 392-399 only check raw
hostnameandsysName. When import naming usesuse_sysname+strip_domain, the actual imported device name may differ from these raw values. This can miss a just-imported object and allow duplicate import attempts.Consider checking
validation.get("resolved_name")(or the computed name from naming options) before falling back to raw hostname/sysName.💡 Proposed fix
librenms_id = libre_device.get("device_id") hostname = libre_device.get("hostname", "") sys_name = libre_device.get("sysName", "") + resolved_name = validation.get("resolved_name") or libre_device.get("_computed_name", "") new_device = None match_type = None # Check by librenms_id custom field first (JSON multi-server format + legacy) if librenms_id: try: new_device = find_by_librenms_id(Model, int(librenms_id), server_key) if new_device: match_type = "librenms_id" except (ValueError, TypeError): pass - # Fall back to hostname match, then sys_name independently - if not new_device and hostname: - new_device = Model.objects.filter(name__iexact=hostname).first() - if new_device: - match_type = "hostname" - if not new_device and sys_name: - new_device = Model.objects.filter(name__iexact=sys_name).first() - if new_device: - match_type = "sysname" + # Fall back to resolved name first, then raw hostname/sysName + for candidate, candidate_type in ( + (resolved_name, "resolved_name"), + (hostname, "hostname"), + (sys_name, "sysname"), + ): + if new_device or not candidate: + continue + new_device = Model.objects.filter(name__iexact=candidate).first() + if new_device: + match_type = candidate_type + break🤖 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 391 - 399, The fallback lookup in the bulk import block currently only checks raw hostname and sys_name (variables new_device, hostname, sys_name, Model, match_type) which can miss a just-imported device when naming rules (use_sysname/strip_domain) changed the stored name; update the fallback to first check validation.get("resolved_name") (or the computed resolved name from the import naming options) against Model.name (case-insensitive) and set match_type accordingly (e.g., "resolved_name") before falling back to raw hostname/sys_name, so newly imported objects are re-checked by their actual stored name.netbox_librenms_plugin/tables/modules.py (1)
201-201: 🧹 Nitpick | 🔵 TrivialSimplify redundant nested
format_htmlcall.Line 201 wraps
format_htmltwice; a single call is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/modules.py` at line 201, The return statement in the function that renders module buttons uses a redundant nested format_html call; replace the nested call so you call format_html only once with the joined button strings (i.e., use format_html("{}", "".join(str(b) for b in buttons))) and keep the buttons falsy branch returning "" unchanged; update the return at the location where buttons is used (the current return expression with format_html("{}", format_html("".join(...)))) to the single format_html invocation.netbox_librenms_plugin/views/base/modules_view.py (2)
741-1070:⚠️ Potential issue | 🟠 MajorMove sync-action views out of
views/baseinto the sync layer.
InstallModuleViewandInstallBranchVieware POST sync-action handlers and should not live in the base-view module.As per coding guidelines: “Views must follow a three-layer structure: Base views in
views/base/, Object sync views inviews/object_sync/, and Sync action views inviews/sync/...”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 741 - 1070, The two POST sync-action view classes InstallModuleView and InstallBranchView belong in the sync layer, not views/base; move their class definitions (including their helper methods _collect_branch, _collect_children, _get_module_types, _install_single, _find_parent_module_id, _match_bay) into the appropriate sync folder (views/sync/ or views/object_sync/ per project convention) and delete them from views/base/modules_view.py; after moving, update their module imports and any URL or reverse() references to point to the new module path, ensure the view classes keep their original base mixins (LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, CacheMixin) and keep method names (post, _install_single, etc.) unchanged so existing tests/URL patterns only need import path updates.
375-387:⚠️ Potential issue | 🔴 CriticalAdd cycle protection to recursive inventory traversal.
Line 375 and Line 889 recurse without a visited-set guard. A cyclic
entPhysicalContainedInchain can recurse indefinitely and fail requests.Proposed hardening
- def _collect_descendants(self, parent_idx, inventory_data, depth, results): + def _collect_descendants(self, parent_idx, inventory_data, depth, results, visited=None): + if visited is None: + visited = set() + if parent_idx in visited: + return + visited.add(parent_idx) children = [i for i in inventory_data if i.get("entPhysicalContainedIn") == parent_idx] for child in children: model = (child.get("entPhysicalModelName") or "").strip() if model and model not in _GENERIC_CONTAINER_MODELS: results.append((depth, child)) - self._collect_descendants(child["entPhysicalIndex"], inventory_data, depth + 1, results) + self._collect_descendants(child["entPhysicalIndex"], inventory_data, depth + 1, results, visited) else: - self._collect_descendants(child["entPhysicalIndex"], inventory_data, depth, results) + self._collect_descendants(child["entPhysicalIndex"], inventory_data, depth, results, visited) - def _collect_children(self, parent_idx, inventory_data, items): + def _collect_children(self, parent_idx, inventory_data, items, visited=None): + if visited is None: + visited = set() + if parent_idx in visited: + return + visited.add(parent_idx) children = [i for i in inventory_data if i.get("entPhysicalContainedIn") == parent_idx] for child in children: model = (child.get("entPhysicalModelName") or "").strip() if model: items.append(child) - self._collect_children(child["entPhysicalIndex"], inventory_data, items) + self._collect_children(child["entPhysicalIndex"], inventory_data, items, visited)Also applies to: 889-897
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 375 - 387, The recursion in _collect_descendants can loop on cyclic entPhysicalContainedIn links; add a visited-set guard (e.g., a parameter visited: set[int]) that tracks entPhysicalIndex values, check if child["entPhysicalIndex"] is already in visited before processing/recursing, add the index to visited when you descend, and pass the same visited set into both recursive calls (the deeper and the same-depth branch); also apply the same visited-set pattern to the other recursive inventory traversal function elsewhere in this module that performs similar entPhysicalContainedIn recursion so every initial call starts with an empty set.netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
109-112:⚠️ Potential issue | 🟡 MinorUse an explicit
Nonecheck for LibreNMS IDs.Line 111 uses
if lib_id:, which drops falsy-but-valid IDs. Useif lib_id is not None:to avoid silent misses ininterfaces_by_librenms_id.Proposed fix
- if lib_id: + if lib_id is not None: interfaces_by_librenms_id[str(lib_id)] = interface🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/ip_addresses_view.py` around lines 109 - 112, The loop over all_interfaces uses a falsy check "if lib_id:" which will drop valid IDs like 0 or empty-string; change the condition to an explicit None check by testing "lib_id is not None" after calling get_librenms_device_id(interface, server_key) so valid but falsy IDs are included in interfaces_by_librenms_id; update the condition around the lib_id assignment in the for interface in all_interfaces loop that populates interfaces_by_librenms_id.
🤖 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/virtual_chassis.py`:
- Line 383: The domain construction uses libre_device.get('device_id',
master_device.pk) which doesn't handle device_id == None; change the fallback to
use the actual value when truthy and master_device.pk when device_id is
None/falsey (e.g., replace the expression inside domain=f"librenms-..." with a
conditional fallback such as libre_device.get('device_id') or master_device.pk).
Update the domain assignment where domain=f"librenms-{...}" (in the virtual
chassis/domain-building code that references libre_device and master_device) so
it never produces "librenms-None".
- Around line 243-252: The function _load_vc_member_name_pattern currently uses
LibreNMSSettings.objects.first(), which picks a global record and breaks
multi-server setups; replace that lookup with the multi-server-aware helper in
librenms_api (the module that “handles multi-server configs via LibreNMSSettings
model and caching”) — e.g., import and call the helper (such as
get_active_librenms_settings or the cached settings accessor in librenms_api) to
obtain the active LibreNMSSettings instance, then use
settings.vc_member_name_pattern if present else "-M{position}", preserving the
existing try/except and logger.warning behavior; reference LibreNMSSettings and
_load_vc_member_name_pattern to locate the change.
In `@netbox_librenms_plugin/models.py`:
- Around line 265-271: NormalizationRule.clean() currently only compiles
match_pattern and ignores validating the replacement template, causing runtime
failures in utils.py; update NormalizationRule.clean to also validate
self.replacement using the same template-check logic as
LibreNMSModuleBayAssignment.clean (i.e., attempt to format/validate the
replacement template or run a safe dummy substitution to ensure it won't raise
errors), and raise ValidationError({"replacement": "<message>"} ) when invalid;
reference the NormalizationRule.clean method and the replacement attribute when
implementing this validation.
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 12-30: The table is missing the required selection column; add a
ToggleColumn to the table class (e.g., alongside the existing columns like name,
model, actions) using ToggleColumn(attrs={'input': {'name': 'select'}}) so it
follows the standard selection pattern; update the class to declare this
ToggleColumn (before other columns or next to name) and ensure it's included in
the table definition to satisfy the table contract.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 782-789: After a successful module install (the try block that
calls messages.success with f"Installed {module_type.model} in {module_bay.name}
(serial: {serial or 'N/A'})."), invalidate the device inventory cache before
returning/redirecting: use Django's cache API (e.g., from django.core.cache
import cache) and delete the inventory key for this device (for example
cache.delete(f"inventory:{pk}") or call the existing inventory cache-key helper
if one exists). Add the same cache.delete call in the other install-success
block around lines 864-873 so the modules tab isn't stale after installs.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/vm_operations.py`:
- Around line 16-68: The create_vm_from_librenms function currently inlines
custom_field_data for librenms_id; instead, create the VirtualMachine as before
but omit constructing librenms_id inline and then call the existing helper
set_librenms_device_id(vm, libre_device["device_id"], server_key) from utils.py
to set the custom field (the helper will validate/convert/migrate and save
appropriately); reference the function names create_vm_from_librenms and
set_librenms_device_id so you can locate and update the code accordingly.
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 391-399: The fallback lookup in the bulk import block currently
only checks raw hostname and sys_name (variables new_device, hostname, sys_name,
Model, match_type) which can miss a just-imported device when naming rules
(use_sysname/strip_domain) changed the stored name; update the fallback to first
check validation.get("resolved_name") (or the computed resolved name from the
import naming options) against Model.name (case-insensitive) and set match_type
accordingly (e.g., "resolved_name") before falling back to raw
hostname/sys_name, so newly imported objects are re-checked by their actual
stored name.
In `@netbox_librenms_plugin/tables/modules.py`:
- Line 201: The return statement in the function that renders module buttons
uses a redundant nested format_html call; replace the nested call so you call
format_html only once with the joined button strings (i.e., use
format_html("{}", "".join(str(b) for b in buttons))) and keep the buttons falsy
branch returning "" unchanged; update the return at the location where buttons
is used (the current return expression with format_html("{}",
format_html("".join(...)))) to the single format_html invocation.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 109-112: The loop over all_interfaces uses a falsy check "if
lib_id:" which will drop valid IDs like 0 or empty-string; change the condition
to an explicit None check by testing "lib_id is not None" after calling
get_librenms_device_id(interface, server_key) so valid but falsy IDs are
included in interfaces_by_librenms_id; update the condition around the lib_id
assignment in the for interface in all_interfaces loop that populates
interfaces_by_librenms_id.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 741-1070: The two POST sync-action view classes InstallModuleView
and InstallBranchView belong in the sync layer, not views/base; move their class
definitions (including their helper methods _collect_branch, _collect_children,
_get_module_types, _install_single, _find_parent_module_id, _match_bay) into the
appropriate sync folder (views/sync/ or views/object_sync/ per project
convention) and delete them from views/base/modules_view.py; after moving,
update their module imports and any URL or reverse() references to point to the
new module path, ensure the view classes keep their original base mixins
(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, CacheMixin) and keep
method names (post, _install_single, etc.) unchanged so existing tests/URL
patterns only need import path updates.
- Around line 375-387: The recursion in _collect_descendants can loop on cyclic
entPhysicalContainedIn links; add a visited-set guard (e.g., a parameter
visited: set[int]) that tracks entPhysicalIndex values, check if
child["entPhysicalIndex"] is already in visited before processing/recursing, add
the index to visited when you descend, and pass the same visited set into both
recursive calls (the deeper and the same-depth branch); also apply the same
visited-set pattern to the other recursive inventory traversal function
elsewhere in this module that performs similar entPhysicalContainedIn recursion
so every initial call starts with an empty set.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 247-249: The current guard prevents MAC updates for non-device
interfaces by checking is_device_interface before calling handle_mac_address; if
VM interface table/compare paths still rely on MAC fields, remove the
is_device_interface condition so MACs are handled for all interfaces unless
explicitly excluded via exclude_columns. Concretely, in the block containing
exclude_columns, is_device_interface, librenms_interface.get("ifPhysAddress")
and the call to self.handle_mac_address(interface, ifPhysAddress), change the if
to only check "mac_address" not in exclude_columns (i.e., drop the
is_device_interface check) so librenms_interface ifPhysAddress is processed for
VM interfaces as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28bb32fb-71ce-4e8b-b8f4-e8fe573b3762
📒 Files selected for processing (11)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping.htmlnetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/interfaces.py
Comment on lines
+243
to
+252
| def _load_vc_member_name_pattern() -> str: | ||
| """Load the VC member name pattern from settings, with fallback to default.""" | ||
| from ..models import LibreNMSSettings | ||
|
|
||
| try: | ||
| settings = LibreNMSSettings.objects.first() | ||
| return settings.vc_member_name_pattern if settings else "-M{position}" | ||
| except Exception as e: | ||
| logger.warning(f"Could not load VC member name pattern from settings: {e}. Using default.") | ||
| return "-M{position}" |
There was a problem hiding this comment.
Scope VC naming pattern lookup to the active LibreNMS server, not global .first().
Using LibreNMSSettings.objects.first() can apply the wrong naming pattern in multi-server setups.
💡 Suggested refactor
-def _load_vc_member_name_pattern() -> str:
+def _load_vc_member_name_pattern(server_key: str | None = None) -> str:
"""Load the VC member name pattern from settings, with fallback to default."""
from ..models import LibreNMSSettings
try:
- settings = LibreNMSSettings.objects.first()
+ qs = LibreNMSSettings.objects
+ settings = qs.filter(server_key=server_key).first() if server_key else qs.first()
return settings.vc_member_name_pattern if settings else "-M{position}"
except Exception as e:
logger.warning(f"Could not load VC member name pattern from settings: {e}. Using default.")
return "-M{position}"- vc_name_pattern = _load_vc_member_name_pattern() if master_name else None
+ vc_name_pattern = (
+ _load_vc_member_name_pattern(getattr(api, "server_key", None))
+ if master_name
+ else None
+ )As per coding guidelines, "librenms_api.py ... handles multi-server configs via LibreNMSSettings model and caching."
🤖 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 243 -
252, The function _load_vc_member_name_pattern currently uses
LibreNMSSettings.objects.first(), which picks a global record and breaks
multi-server setups; replace that lookup with the multi-server-aware helper in
librenms_api (the module that “handles multi-server configs via LibreNMSSettings
model and caching”) — e.g., import and call the helper (such as
get_active_librenms_settings or the cached settings accessor in librenms_api) to
obtain the active LibreNMSSettings instance, then use
settings.vc_member_name_pattern if present else "-M{position}", preserving the
existing try/except and logger.warning behavior; reference LibreNMSSettings and
_load_vc_member_name_pattern to locate the change.
Comment on lines
+782
to
+789
| messages.success( | ||
| request, f"Installed {module_type.model} in {module_bay.name} (serial: {serial or 'N/A'})." | ||
| ) | ||
| except Exception as e: | ||
| messages.error(request, f"Failed to install module: {e}") | ||
|
|
||
| sync_url = reverse("plugins:netbox_librenms_plugin:device_librenms_sync", kwargs={"pk": pk}) | ||
| return redirect(f"{sync_url}?tab=modules#librenms-module-table") |
There was a problem hiding this comment.
Invalidate cached inventory after successful installs.
After installation, the code redirects without clearing inventory cache. The modules tab can render stale status until a manual refresh.
Proposed fix
class InstallModuleView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, View):
@@
messages.success(
request, f"Installed {module_type.model} in {module_bay.name} (serial: {serial or 'N/A'})."
)
+ cache.delete(f"librenms_inventory_device_{device.pk}")
@@
class InstallBranchView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, CacheMixin, View):
@@
if installed:
messages.success(request, f"Installed {len(installed)} module(s): {', '.join(installed)}")
+ cache.delete(self.get_cache_key(device, "inventory"))Also applies to: 864-873
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 782 - 789,
After a successful module install (the try block that calls messages.success
with f"Installed {module_type.model} in {module_bay.name} (serial: {serial or
'N/A'})."), invalidate the device inventory cache before returning/redirecting:
use Django's cache API (e.g., from django.core.cache import cache) and delete
the inventory key for this device (for example cache.delete(f"inventory:{pk}")
or call the existing inventory cache-key helper if one exists). Add the same
cache.delete call in the other install-success block around lines 864-873 so the
modules tab isn't stale after installs.
… guard, IP id check, migrate conflict, MAC sync, NormRule clean, cycle guard, move install views
- virtual_chassis.py: fix 'librenms-None' domain (use .get() or fallback),
preload vc_pattern before master rename to avoid double DB read,
add order_by('pk') to _load_vc_member_name_pattern for determinism
- vm_operations.py: use set_librenms_device_id() instead of inline CF dict
- librenms_sync_base.html: guard librenms_server_info existence before .is_legacy
- ip_addresses_view.py: use 'if lib_id is not None:' instead of 'if lib_id:'
- actions.py: add conflict check for duplicate librenms_id in migrate_librenms_id
- interfaces.py: remove is_device_interface guard from MAC handling,
add hasattr check so VMInterface (no primary_mac_address) is handled safely
- models.py: NormalizationRule.clean validates replacement template via dummy sub
- modules_view.py: add visited-set cycle guard to _collect_descendants and _collect_children
- tables/modules.py: replace redundant nested format_html with mark_safe
- views/sync/modules.py: move InstallModuleView and InstallBranchView out of
views/base into views/sync (correct 3-layer placement)
- views/__init__.py: update imports to point to new sync.modules location
- tests: update mocks for order_by().first() chain; update VM creation test
…port resolved_name fallback - views/sync/modules.py: invalidate inventory cache after successful module install (InstallModuleView) and branch install (InstallBranchView) - import_utils/bulk_import.py: check validation resolved_name before raw hostname/sysname in post-import device lookup fallback
- test_view_wiring: smoke tests for mixin/MRO wiring on all sync views - test_librenms_id: get_librenms_device_id, find_by_librenms_id, migrate_legacy, roundtrip - test_mixins: LibreNMSAPIMixin lazy init + get_server_info, CacheMixin key generation - test_sync_interfaces: update_interface_attributes (all branches), handle_mac_address - test_sync_modules: InstallBranchView branch collection, cycle guard, wiring (inventory-rebased) - test_sync_devices: AddDeviceToLibreNMSView, UpdateDeviceLocationView, field view wiring - mock_librenms_server: reusable HTTP mock for integration tests - test_integration_sync: end-to-end API call tests via mock HTTP server
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
netbox_librenms_plugin/views/imports/actions.py (1)
1251-1257:⚠️ Potential issue | 🟠 Major
migrate_librenms_idconflict guard still misses legacy integer owners.This branch only checks
custom_field_data__librenms_id__{server_key}. It does not check devices still on legacy integer format (custom_field_data__librenms_id=<id>), so mixed-format duplicate ownership can still slip through during migration.Proposed fix
with transaction.atomic(): + from django.db.models import Q try: locked_device = Device.objects.select_for_update().get(pk=existing_device.pk) except Device.DoesNotExist: return HttpResponse( "Device no longer exists; it may have been deleted concurrently.", status=409, ) @@ # Check that no other device already owns this ID on this server server_key = self.librenms_api.server_key conflict = ( - Device.objects.filter(**{f"custom_field_data__librenms_id__{server_key}": cf_locked}) + Device.objects.filter( + Q(**{f"custom_field_data__librenms_id__{server_key}": cf_locked}) + | Q(custom_field_data__librenms_id=cf_locked) + ) .exclude(pk=locked_device.pk) .exists() ) if conflict: return HttpResponse(🤖 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 1251 - 1257, The conflict check in migrate_librenms_id only queries the namespaced key custom_field_data__librenms_id__{server_key} and misses legacy integer owners stored under custom_field_data__librenms_id, so update the Device ownership query (the block that builds conflict using server_key and cf_locked) to check both formats: include devices where custom_field_data__librenms_id__{server_key} == cf_locked OR custom_field_data__librenms_id == cf_locked (use a Q() OR to combine filters and keep the .exclude(pk=locked_device.pk).exists() semantics), ensuring both new namespaced and legacy integer entries are considered.netbox_librenms_plugin/views/base/modules_view.py (1)
369-370:⚠️ Potential issue | 🟡 MinorSeed
visitedwith the parent index to avoid parent re-insertion in cyclic graphs.Current logic prevents infinite recursion, but a malformed cycle can still append the original parent as a descendant once.
💡 Suggested patch
def _get_sub_components(self, parent_idx, inventory_data): """Find descendant items with a model name (real hardware, not empty containers). Returns list of (depth, item) tuples. """ results = [] - self._collect_descendants(parent_idx, inventory_data, depth=1, results=results, visited=set()) + self._collect_descendants(parent_idx, inventory_data, depth=1, results=results, visited={parent_idx}) return results def _collect_descendants(self, parent_idx, inventory_data, depth, results, visited=None): """Recursively collect descendant items that have a model name.""" if visited is None: visited = set() + if parent_idx in visited: + return + visited.add(parent_idx) children = [i for i in inventory_data if i.get("entPhysicalContainedIn") == parent_idx] for child in children: child_idx = child["entPhysicalIndex"] if child_idx in visited: continue visited.add(child_idx)Also applies to: 372-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 369 - 370, The recursive descendant collection currently calls self._collect_descendants(parent_idx, inventory_data, depth=1, results=results, visited=set()) which uses an empty visited set and allows the parent to be re-added in malformed cycles; change the call sites (the initial call(s) that pass visited=set()) to seed visited with the parent index (e.g., visited={parent_idx}) so the parent is marked visited before recursion begins, preventing parent re-insertion in cyclic graphs while preserving the existing infinite-recursion guard inside _collect_descendants.netbox_librenms_plugin/tables/modules.py (1)
12-30:⚠️ Potential issue | 🟠 MajorAdd the standard selection
ToggleColumnto keep table behavior consistent.This table omits the required selection-column pattern used by sync tables, which can break shared selection/bulk-action expectations.
As per coding guidelines:
**/tables/**/*.py: “Table classes intables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection...”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/modules.py` around lines 12 - 30, The table is missing the required selection ToggleColumn used by sync/bulk actions; add a ToggleColumn(attrs={'input': {'name': 'select'}}) as the first column on this table (alongside the existing columns such as name, model, serial, actions) and import ToggleColumn from the table library used (e.g., django_tables2.ToggleColumn) so the table adheres to the selection pattern expected by shared bulk-action code.
🤖 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/bulk_import.py`:
- Around line 527-529: The status check uses job.job.status directly which may
be an enum object; normalize it first (e.g., status = job.job.status.value if
hasattr(job.job.status, "value") else job.job.status) and then compare that
normalized status against the known JobStatusChoices values (use their .value or
string literals as appropriate) before deciding cancellation/early-exit; apply
this normalization in the block around job.job in bulk_import.py (the check
using JobStatusChoices.STATUS_FAILED/STATUS_ERRORED and the similar check later
at the other occurrence) so comparisons are consistent whether status is an enum
or a raw string.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html`:
- Around line 641-646: The module tab include uses the wrong path and breaks the
sync-template convention; inside the module_sync conditional (the `{% if
module_sync %}` block) replace the include
`'netbox_librenms_plugin/inc/_module_sync.html'` with the sync-wrapper style
path `'netbox_librenms_plugin/_module_sync.html'` so the template follows the
`_<resource>_sync.html` convention used by other sync wrappers and the module
tab renders correctly.
In `@netbox_librenms_plugin/tests/test_integration_sync.py`:
- Around line 20-41: The helper _make_api sets librenms_url and api_token twice:
LibreNMSAPI(server_key="test") already initializes api.librenms_url and
api.api_token from the patched get_plugin_config; remove the redundant manual
assignments to api.librenms_url and api.api_token at the end of _make_api so the
function returns the API instance as configured by LibreNMSAPI.__init__ (keep
the patch of get_plugin_config and creation of LibreNMSAPI unchanged).
In `@netbox_librenms_plugin/tests/test_mixins.py`:
- Around line 111-162: The tests (test_get_cache_key_format,
test_get_cache_key_includes_model_name, test_get_cache_key_different_data_types,
test_get_last_fetched_key_format, test_cache_key_different_pks_differ) create
ad-hoc MagicMock objects but should reuse the existing mock_netbox_device
fixture from tests/conftest.py for consistency; update each test to accept and
use the mock_netbox_device fixture (or a parametrized variant) instead of
constructing MagicMock() and set/override its _meta.model_name and pk as needed,
while still calling mixin.get_cache_key(...) and mixin.get_last_fetched_key(...)
to assert the same expectations.
- Around line 164-173: The test
test_get_vlan_overrides_key_exists_and_differs_from_data_key uses a hasattr
guard that lets the test silently pass if get_vlan_overrides_key is missing;
change it to explicitly require the method by asserting hasattr(mixin,
"get_vlan_overrides_key") (or decorate the test with pytest.mark.skipif when the
mixin legitimately lacks that API) before calling
mixin.get_vlan_overrides_key(obj), then compare vlan_key to data_key using
mixin.get_cache_key(obj, "vlans"); update references in the test to ensure
get_vlan_overrides_key and get_cache_key are explicitly checked rather than
conditionally skipped.
In `@netbox_librenms_plugin/tests/test_sync_modules.py`:
- Around line 108-149: The test currently builds mocks but never calls the real
implementation—replace the manual indexing block with a real invocation of the
target method: patch dcim.models.ModuleType and
netbox_librenms_plugin.models.ModuleTypeMapping so that
ModuleType.objects.all().select_related() returns [mt1, mt2] and
ModuleTypeMapping.objects.select_related().return_value returns [mock_mapping],
then import or instantiate the view that defines _get_module_types (referencing
_get_module_types and InstallBranchView) and call view._get_module_types();
finally assert the returned dict keys map to mt1/mt2 as before (check
"WS-X4748", "ALT-PART-4748", "WS-X4516", and mock_mapping.librenms_model).
Ensure the same sys.modules patch used earlier is present so the inline import
inside _get_module_types resolves to the patched ModuleType.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 267-308: The N+1 query comes from
ModuleBay.objects.filter(device=device) and ModuleBayMapping.objects.filter(...)
being executed inside _find_parent_module_id for each loop/ item; refactor by
changing _find_parent_module_id to accept pre-fetched collections (e.g.,
device_bays queryset/list and module_mappings dict/list) and update callers
(notably _install_single) to fetch
ModuleBay.objects.filter(device=device).select_related("installed_module") once
and ModuleBayMapping.objects.all() once (or convert mappings to a dict keyed by
librenms_name) and pass them into _find_parent_module_id so the method uses the
provided in-memory collections instead of running ORM queries in the loop.
- Around line 17-66: InstallModuleView uses a hardcoded cache key when deleting
the LibreNMS inventory cache
(cache.delete(f"librenms_inventory_device_{device.pk}")) while InstallBranchView
uses CacheMixin and self.get_cache_key(device, "inventory"); update
InstallModuleView to inherit CacheMixin and replace the hardcoded key with
cache.delete(self.get_cache_key(device, "inventory")) to ensure consistent cache
key management and maintainability (modify the class definition for
InstallModuleView to include CacheMixin and the cache.delete call in the post
method).
---
Duplicate comments:
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 12-30: The table is missing the required selection ToggleColumn
used by sync/bulk actions; add a ToggleColumn(attrs={'input': {'name':
'select'}}) as the first column on this table (alongside the existing columns
such as name, model, serial, actions) and import ToggleColumn from the table
library used (e.g., django_tables2.ToggleColumn) so the table adheres to the
selection pattern expected by shared bulk-action code.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 369-370: The recursive descendant collection currently calls
self._collect_descendants(parent_idx, inventory_data, depth=1, results=results,
visited=set()) which uses an empty visited set and allows the parent to be
re-added in malformed cycles; change the call sites (the initial call(s) that
pass visited=set()) to seed visited with the parent index (e.g.,
visited={parent_idx}) so the parent is marked visited before recursion begins,
preventing parent re-insertion in cyclic graphs while preserving the existing
infinite-recursion guard inside _collect_descendants.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1251-1257: The conflict check in migrate_librenms_id only queries
the namespaced key custom_field_data__librenms_id__{server_key} and misses
legacy integer owners stored under custom_field_data__librenms_id, so update the
Device ownership query (the block that builds conflict using server_key and
cf_locked) to check both formats: include devices where
custom_field_data__librenms_id__{server_key} == cf_locked OR
custom_field_data__librenms_id == cf_locked (use a Q() OR to combine filters and
keep the .exclude(pk=locked_device.pk).exists() semantics), ensuring both new
namespaced and legacy integer entries are considered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20328d07-a68e-47b8-85df-f40ed7d0bdb2
📒 Files selected for processing (22)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_interfaces.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/modules.py
Comment on lines
527
to
529
| job.job.refresh_from_db() | ||
| if job.job.status == JobStatusChoices.STATUS_FAILED: | ||
| if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED): | ||
| job.logger.warning("Job was stopped before validation started") |
There was a problem hiding this comment.
Normalize DB job status before cancellation comparisons.
These fallback checks compare job.job.status directly. If status is enum-like (with .value), cancellation may be missed and processing may continue unexpectedly.
💡 Suggested patch
- job.job.refresh_from_db()
- if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED):
+ job.job.refresh_from_db()
+ status = job.job.status
+ status_value = status.value if hasattr(status, "value") else status
+ if status_value in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED, "failed", "errored"):
job.logger.warning("Job was stopped before validation started")
return _empty_return(return_cache_status)
@@
- job.job.refresh_from_db()
- if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED):
+ job.job.refresh_from_db()
+ status = job.job.status
+ status_value = status.value if hasattr(status, "value") else status
+ if status_value in (
+ JobStatusChoices.STATUS_FAILED,
+ JobStatusChoices.STATUS_ERRORED,
+ "failed",
+ "errored",
+ ):
job.logger.info(f"Job stopped at device {idx}/{total}. Exiting gracefully.")
return _empty_return(return_cache_status)Based on learnings: Applies to /views/imports/,**/import_utils.py : Recognize database Job status values as pending, scheduled, running, completed, failed, errored (NO cancelled status exists).
Also applies to: 557-559
🤖 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 527 - 529,
The status check uses job.job.status directly which may be an enum object;
normalize it first (e.g., status = job.job.status.value if
hasattr(job.job.status, "value") else job.job.status) and then compare that
normalized status against the known JobStatusChoices values (use their .value or
string literals as appropriate) before deciding cancellation/early-exit; apply
this normalization in the block around job.job in bulk_import.py (the check
using JobStatusChoices.STATUS_FAILED/STATUS_ERRORED and the similar check later
at the other occurrence) so comparisons are consistent whether status is an enum
or a raw string.
Comment on lines
+111
to
+162
| def test_get_cache_key_format(self): | ||
| mixin = self._make_mixin() | ||
| obj = MagicMock() | ||
| obj._meta.model_name = "device" | ||
| obj.pk = 5 | ||
|
|
||
| key = mixin.get_cache_key(obj, "ports") | ||
| assert key == "librenms_ports_device_5" | ||
|
|
||
| def test_get_cache_key_includes_model_name(self): | ||
| mixin = self._make_mixin() | ||
| obj = MagicMock() | ||
| obj._meta.model_name = "virtualmachine" | ||
| obj.pk = 10 | ||
|
|
||
| key = mixin.get_cache_key(obj, "interfaces") | ||
| assert "virtualmachine" in key | ||
| assert "10" in key | ||
|
|
||
| def test_get_cache_key_different_data_types(self): | ||
| mixin = self._make_mixin() | ||
| obj = MagicMock() | ||
| obj._meta.model_name = "device" | ||
| obj.pk = 1 | ||
|
|
||
| key_ports = mixin.get_cache_key(obj, "ports") | ||
| key_ips = mixin.get_cache_key(obj, "ips") | ||
| assert key_ports != key_ips | ||
|
|
||
| def test_get_last_fetched_key_format(self): | ||
| mixin = self._make_mixin() | ||
| obj = MagicMock() | ||
| obj._meta.model_name = "device" | ||
| obj.pk = 3 | ||
|
|
||
| key = mixin.get_last_fetched_key(obj, "ports") | ||
| # Should include "last_fetched" and the object identifiers | ||
| assert "last_fetched" in key | ||
| assert "device" in key | ||
| assert "3" in key | ||
|
|
||
| def test_cache_key_different_pks_differ(self): | ||
| mixin = self._make_mixin() | ||
| obj1 = MagicMock() | ||
| obj1._meta.model_name = "device" | ||
| obj1.pk = 1 | ||
|
|
||
| obj2 = MagicMock() | ||
| obj2._meta.model_name = "device" | ||
| obj2.pk = 2 | ||
|
|
||
| assert mixin.get_cache_key(obj1, "ports") != mixin.get_cache_key(obj2, "ports") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reusing fixtures from conftest.py for mock objects.
The tests create ad-hoc MagicMock objects for NetBox devices. Per testing guidelines, fixtures like mock_netbox_device from tests/conftest.py should be reused when available to maintain consistency across the test suite.
If these cache key tests require minimal object structure (just _meta.model_name and pk), the current approach is acceptable, but worth noting for consistency. Based on learnings: "Reuse fixtures from tests/conftest.py instead of creating ad-hoc mocks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tests/test_mixins.py` around lines 111 - 162, The
tests (test_get_cache_key_format, test_get_cache_key_includes_model_name,
test_get_cache_key_different_data_types, test_get_last_fetched_key_format,
test_cache_key_different_pks_differ) create ad-hoc MagicMock objects but should
reuse the existing mock_netbox_device fixture from tests/conftest.py for
consistency; update each test to accept and use the mock_netbox_device fixture
(or a parametrized variant) instead of constructing MagicMock() and set/override
its _meta.model_name and pk as needed, while still calling
mixin.get_cache_key(...) and mixin.get_last_fetched_key(...) to assert the same
expectations.
- Add ToggleColumn to LibreNMSModuleTable for per-row selection - Wrap module sync table in a form that POSTs to install_selected URL - Add InstallSelectedView that reuses InstallBranchView._install_single - Register install_selected URL and export from views __init__ - Complete test_sync_modules.py: bay matching, serial comparison, depth, _install_single status paths (52 tests total) - Complete test_integration_sync.py: add TestLibreNMSAPIInventory (4 tests) - Fix all patch paths for inline-imported utils to source module - Patch transaction.atomic for _install_single unit tests
…heck, tests
- librenms_sync_base.html: use _module_sync.html top-level path (follow convention)
- views/sync/modules.py: add CacheMixin to InstallModuleView, use get_cache_key
- views/sync/modules.py: refactor _find_parent_module_id to accept pre-fetched
device_bays/bay_mappings; update _install_single to pre-fetch and pass them,
eliminating N+1 DB queries in the parent-module lookup loop
- views/base/modules_view.py: seed visited={parent_idx} in _get_descendants to
prevent self-referencing items being added as their own descendants
- views/imports/actions.py: add Q OR to migrate_librenms_id conflict check so
both namespaced and legacy integer librenms_id owners are detected
- tests/test_mixins.py: change hasattr guard to explicit assert for vlan_overrides_key
- tests/test_integration_sync.py: remove redundant url/token assignments in _make_api
- tests/test_sync_modules.py: call real _get_module_types(); patch ModuleBayMapping
in _install_single tests to avoid DB access
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
netbox_librenms_plugin/tests/test_mixins.py (1)
111-162: 🛠️ Refactor suggestion | 🟠 MajorReuse
tests/conftest.pyfixtures for NetBox objects in cache-key tests.These tests still create ad-hoc model mocks repeatedly; please switch to
mock_netbox_device(overridingpk/_meta.model_nameper case) for consistency with the suite’s testing standard.Based on learnings: "Reuse fixtures from
tests/conftest.pyinstead of creating ad-hoc mocks."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_mixins.py` around lines 111 - 162, Replace the ad-hoc MagicMock instances in the cache-key tests with the shared fixture mock_netbox_device (overriding its pk and _meta.model_name as needed) so the tests use the standard NetBox object fixture; update tests calling get_cache_key and get_last_fetched_key (e.g., test_get_cache_key_format, test_get_cache_key_includes_model_name, test_get_cache_key_different_data_types, test_get_last_fetched_key_format, test_cache_key_different_pks_differ) to accept or obtain mock_netbox_device and set mock_netbox_device.pk and mock_netbox_device._meta.model_name per case instead of constructing MagicMock objects.
🤖 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/tables/modules.py`:
- Around line 134-142: The tooltip string is currently a literal "{module_path}"
and never interpolated; update the format_html call in modules.py (the block
that checks record.get("module_path_warning")) to use a format placeholder and
pass the actual module path value (e.g., record.get("module_path") or
module_path) as an additional argument—for example change the final argument to
a format string like "Upgrade NetBox to fully support {}" and supply the module
path so the tooltip shows the real module path instead of the literal
placeholder.
- Around line 38-40: Meta.row_attrs currently only sets the CSS class; add the
required row-level data-* attributes here so sync-table behavior can read them.
Update the Meta class's row_attrs (the lambda currently referenced by row_attrs)
to return a dict that includes the existing "class" plus the needed data-* keys
(e.g., "data-id", "data-type" or whatever the sync-table contract expects)
populated from the record via record.get(...); ensure the lambda still falls
back to empty strings when keys are missing.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html`:
- Around line 19-32: The outer form that posts to the install_selected view
(action using {% url 'plugins:netbox_librenms_plugin:install_selected'
pk=module_sync.object.pk %}) wraps the table whose rows already contain their
own forms, causing nested/invalid forms and breaking submissions; remove this
parent <form> (and keep the CSRF token and submit button logic out of a form)
and instead replace it with a non-form container element (e.g., a <div>) around
the install-selected button and the card, or move the bulk-install button into a
proper single form that posts only selected IDs; update references to the
install-selected button id "install-selected-btn" and ensure per-row forms
inside module_sync.table remain unchanged so per-row Install/Install Branch
actions continue to work.
In `@netbox_librenms_plugin/tests/test_sync_modules.py`:
- Around line 167-173: The test test_install_module_view_not_in_base currently
only asserts when "netbox_librenms_plugin.views.base.modules_view" is already in
sys.modules, so it can be skipped; explicitly import or reload the module (e.g.,
via importlib.import_module or importlib.reload) to ensure modules_view is
present in sys.modules before checking for InstallModuleView, then assert that
the attribute InstallModuleView is not present on the imported module to
guarantee the assertion always runs; update references in the test to use the
module name "netbox_librenms_plugin.views.base.modules_view" and the class name
InstallModuleView.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 131-132: The ancestor traversal that uses fixed loops (for _ in
range(10) checking current_idx against index_map, and the separate
container-climb loop capped at range(5)) should be replaced with
visited-set-driven while loops: in the code paths around the variables
current_idx and index_map (and the container climb logic referenced later), loop
while current_idx is truthy and present in index_map and not in a visited set,
add current_idx to visited each iteration, advance current_idx to its
parent/container index, and stop when parent is None or a cycle is detected;
remove the hard-coded numeric bounds and ensure you detect cycles via the
visited set to avoid infinite loops.
- Around line 483-492: The ModuleBayMapping lookups inside the candidate/name
loops cause N+1 queries; instead, before iterating build in-memory lookup
structures by preloading ModuleBayMapping rows for the current request: query
ModuleBayMapping once for librenms_name in candidate_names (and include both
librenms_class==phys_class and empty class) and separately preload mappings
where is_regex=True; then inside the loop use a dict keyed by (librenms_name,
librenms_class) for exact matches and run cached regex patterns against
candidate names for regex matches. Apply the same preload-and-match approach to
the other loop referenced (lines 547-552) so both places use the cached
ModuleBayMapping results rather than querying per candidate.
- Around line 98-99: The code builds index_map and elsewhere reads
item["entPhysicalIndex"] directly from external inventory_data which can raise
KeyError for malformed rows; change those direct reads to use
item.get("entPhysicalIndex") and skip or log items where that value is None
before inserting into index_map (e.g., build index_map with {idx: item for item
in inventory_data if (idx := item.get("entPhysicalIndex")) is not None}), and
update any other occurrences that assume the key (the places currently using
item["entPhysicalIndex"]) to tolerate missing keys by checking for None or using
.get and handling the missing-case (skip, warn via logger, or continue) so the
module sync does not abort on malformed rows.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 24-25: Replace the JSON permission checks in the POST handlers so
they return HTML redirects/messages instead of JSON: in InstallModuleView.post,
InstallBranchView.post, and InstallSelectedView.post, change the call from
require_all_permissions_json("POST") to require_all_permissions("POST") and
return that result on failure (i.e., keep the guard pattern but call
require_all_permissions). This ensures the handlers use the correct non-AJAX
permission flow.
- Around line 402-410: The code currently converts selected_indices into a set
(selected_set) which loses user-specified order and makes parent/child install
ordering nondeterministic; change this to preserve order by iterating
selected_indices, converting each element to int (raising the same ValueError
path if any conversion fails) into a list (e.g., selected_list or
selected_indices_int) instead of a set, and then build items using that ordered
list with index_map (items = [index_map[idx] for idx in selected_list if idx in
index_map]); keep the existing error handling that calls messages.error and
redirects when conversion fails.
---
Duplicate comments:
In `@netbox_librenms_plugin/tests/test_mixins.py`:
- Around line 111-162: Replace the ad-hoc MagicMock instances in the cache-key
tests with the shared fixture mock_netbox_device (overriding its pk and
_meta.model_name as needed) so the tests use the standard NetBox object fixture;
update tests calling get_cache_key and get_last_fetched_key (e.g.,
test_get_cache_key_format, test_get_cache_key_includes_model_name,
test_get_cache_key_different_data_types, test_get_last_fetched_key_format,
test_cache_key_different_pks_differ) to accept or obtain mock_netbox_device and
set mock_netbox_device.pk and mock_netbox_device._meta.model_name per case
instead of constructing MagicMock objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcfa62f6-3cde-4222-b198-5806bd457a4f
📒 Files selected for processing (12)
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/modules.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.12)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.13)
🧰 Additional context used
📓 Path-based instructions (11)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow frontend conventions for templates and static files defined in
.github/instructions/frontend.instructions.md
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer for table row updates. Table row updates must return<tr hx-swap-oob="true">. AvoidouterHTMLswaps; use OOB (Out-of-Band) or targetedinnerHTMLswaps to keep table layout intact.
Styling assumes Tabler defaults. Removingtable-responsivewrappers was deliberate to prevent dropdown clipping—do not re-add them.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html
netbox_librenms_plugin/{templates,static}/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/{templates,static}/**/*.{html,js}: All HTMX requests andfetch()calls must include a CSRF token. The standard pattern isdocument.querySelector('[name=csrfmiddlewaretoken]').value(from a hidden form input). The import JS also usesgetCookie('csrftoken')as a fallback — prefer the hidden input approach for consistency.
Modals use Tabler (Bootstrap-like) but withoutbootstrap.Modalhelpers. Buttons target thehtmx-modal-contentelement and JavaScript inlibrenms_import.htmltoggles the wrapper. Do not reintroducedata-bs-toggleor duplicate modal IDs.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html
netbox_librenms_plugin/{templates,tables}/**/*.{html,py}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Templates live in
templates/netbox_librenms_plugin/; reuse/includes underinc/. Sync pages extendlibrenms_sync_base.html. Tables emit HTMX-enabled columns and buttons (tables/*.py), so prefer updating the table renderer in Python rather than templates when changing row actions.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/tables/modules.py
netbox_librenms_plugin/templates/**/_*_sync{,_content}.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Each sync resource has two templates following a naming convention:
_<resource>_sync.html(the tab wrapper, loaded once when the tab is selected) and_<resource>_sync_content.html(the HTMX-swappable inner fragment, refreshed on data changes without a full page reload). Current resources:_interface_sync,_cable_sync,_ipaddress_sync,_vlan_sync. When adding a new sync resource, create both the wrapper and content templates following this pattern.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html
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/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/urls.py
netbox_librenms_plugin/views/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/views/**/*.py: Views must follow a three-layer structure: Base views inviews/base/, Object sync views inviews/object_sync/, and Sync action views inviews/sync/with shared mixins fromviews/mixins.py
New views must extend the closest base class and compose mixins fromviews/mixins.py(LibreNMSPermissionMixin,NetBoxObjectPermissionMixin,LibreNMSAPIMixin,CacheMixin,VlanAssignmentMixin)
All views must inheritLibreNMSPermissionMixinfromviews/mixins.pywithpermission_required = PERM_VIEW_PLUGIN
Declarerequired_object_permissionsas a dict mapping HTTP methods to[(action, Model)]tuples for NetBox model operations; some views may set this dynamically per-request
Use_get_safe_redirect_url(request)to validate referrer URLs in permission checks to prevent open-redirect attacks
Files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.py
netbox_librenms_plugin/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/views/sync/**/*.py: Sync POST handlers must callrequire_all_permissions()(not justrequire_write_permission()) and return early if it returns a response; userequire_all_permissions_json()for AJAX/JSON endpoints
Follow sync conventions defined in.github/instructions/sync.instructions.mdfor sync views, base views, tables, and sync JavaScript
Files:
netbox_librenms_plugin/views/sync/modules.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/modules.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/modules_view.py
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/modules.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: Job cancellation flow: (1) Call/api/core/background-tasks/{uuid}/stop/to stop RQ job, (2) Call plugin's sync endpoint/api/plugins/librenms_plugin/jobs/{pk}/sync-status/to update database, (3) Frontend polling detects status changes and redirects appropriately
Poll/api/core/background-tasks/{uuid}/for real-time RQ status, update modal messages based on status transitions (queued,started,finished,stopped,failed), handle all RQ status values explicitly to avoid infinite polling, and usecancelInProgressflag to prevent polling interference during cancellation
NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet); non-superuser users cannot poll job status and get 403 Forbidden. The plugin must automatically fall back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Custom sync endpointapi/views.py::sync_job_status()must sync database Job status with RQ job status, needed because NetBox worker doesn't always update DB when jobs stop before processing starts
Import page supports synchronous mode (callsprocess_device_filters()directly, renders results inline) and background mode (enqueuesFilterDevicesJob, returnsJsonResponsewithjob_id/job_pk/poll_url, frontend polls and redirects on completion)
Result loading via_load_job_results(job_id)must readjob.data['device_ids']and reconstruct devices from per-device cache usingget_validated_device_cache_key()
Import filter fields must include:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
DeviceImportHelperMixinmust provideget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) must render confirmation modal with selected device...
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (55)
📓 Common learnings
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: Keep REST endpoint server responses and HTMX targets in sync; implement REST endpoints in `views/imports/actions.py` and HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
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: Sync pipelines must follow the pattern: fetch LibreNMS data via `librenms_api.py`, cache with `CacheMixin`, build comparison tables via `tables/*.py`, and render HTMX fragments from `templates/netbox_librenms_plugin/htmx/`
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/_*_sync{,_content}.html : Each sync resource has two templates following a naming convention: `_<resource>_sync.html` (the tab wrapper, loaded once when the tab is selected) and `_<resource>_sync_content.html` (the HTMX-swappable inner fragment, refreshed on data changes without a full page reload). Current resources: `_interface_sync`, `_cable_sync`, `_ipaddress_sync`, `_vlan_sync`. When adding a new sync resource, create both the wrapper and content templates following this pattern.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/{templates,tables}/**/*.{html,py} : Templates live in `templates/netbox_librenms_plugin/`; reuse/includes under `inc/`. Sync pages extend `librenms_sync_base.html`. Tables emit HTMX-enabled columns and buttons (`tables/*.py`), so prefer updating the table renderer in Python rather than templates when changing row actions.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/htmx/*.html : HTMX fragments live in `templates/netbox_librenms_plugin/htmx/` and include: `device_import_row.html` (individual import row updates), `device_validation_details.html` (expandable validation details), `device_vc_details.html` (virtual chassis member details), `bulk_import_confirm.html` (import confirmation modal content). Keep server responses and HTMX targets in sync when modifying these fragments.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.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/views/sync/**/*.py : Follow sync conventions defined in `.github/instructions/sync.instructions.md` for sync views, base views, tables, and sync JavaScript
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/sync/**/*.py : Sync action views must follow the pattern: check permissions with `LibreNMSPermissionMixin` and `NetBoxObjectPermissionMixin`, read selected items from `request.POST.getlist('select')`, load cached data using `CacheMixin.get_cache_key()`, apply changes inside `transaction.atomic()`, and redirect to the sync tab with `?tab=<resource>`.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/{templates,static}/**/*.{html,js} : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers. Buttons target the `htmx-modal-content` element and JavaScript in `librenms_import.html` toggles the wrapper. Do not reintroduce `data-bs-toggle` or duplicate modal IDs.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/tables/modules.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: Keep REST endpoint server responses and HTMX targets in sync; implement REST endpoints in `views/imports/actions.py` and HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : HTMX 2.x is the primary async layer for table row updates. Table row updates must return `<tr hx-swap-oob="true">`. Avoid `outerHTML` swaps; use OOB (Out-of-Band) or targeted `innerHTML` swaps to keep table layout intact.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/htmx/*import*.html : Device import dropdowns rely on TomSelect decorators set up elsewhere. Keep `<select class="device-role-select">` markup stable to preserve JS hook-up.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tables/modules.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/views/**/*.py : Views must follow a three-layer structure: Base views in `views/base/`, Object sync views in `views/object_sync/`, and Sync action views in `views/sync/` with shared mixins from `views/mixins.py`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/settings.html : `settings.html` uses a split-form pattern: two separate Django forms (`ServerConfigForm` + `ImportSettingsForm`) sharing one page, differentiated by a hidden `form_type` field (`"server_config"` or `"import_settings"`). The test-connection button is an HTMX POST to `TestLibreNMSConnectionView`, returning an inline alert fragment.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.html
📚 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: Sync pipelines must follow the pattern: fetch LibreNMS data via `librenms_api.py`, cache with `CacheMixin`, build comparison tables via `tables/*.py`, and render HTMX fragments from `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/tables/modules.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module.
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tests/test_integration_sync.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/test_netbox_librenms_plugin.py : `test_netbox_librenms_plugin.py` is an empty placeholder — do not add tests there.
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_integration_sync.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 : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.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/api/**/*.py : Update API serializers in `api/serializers.py` and `api/views.py` together to prevent contract drift between models and external API consumers
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch `cache` where imported: `netbox_librenms_plugin.views.imports.list.cache`, not `django.core.cache.cache`.
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/sync/modules.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 : Hook into NetBox Django 5 plugin APIs via `navigation.py`, `urls.py`, and `api/` modules under `netbox_librenms_plugin/`; respect NetBox plugin conventions
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Reuse fixtures from `tests/conftest.py` instead of creating ad-hoc mocks: Configuration (`mock_multi_server_config`, `mock_legacy_config`), API client (`mock_librenms_api`), NetBox objects (`mock_netbox_device`, `mock_netbox_vm`, `mock_netbox_site`, `mock_netbox_platform`, `mock_netbox_device_type`, `mock_netbox_device_role`, `mock_netbox_cluster`, `mock_netbox_rack`), HTTP responses (`mock_response_factory`, `mock_success_response`, `mock_device_response`, `mock_error_response`, `mock_auth_error_response`), and Import workflow (`sample_librenms_device`, `sample_librenms_device_minimal`, `sample_validation_state`, `sample_validation_state_vm`).
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_integration_sync.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances.
Applied to files:
netbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_integration_sync.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript URL and tab state management must implement `initializeTabs()`, `getDeviceIdFromUrl()`, and `setInterfaceNameFieldFromURL()` to maintain browser state and URL synchronization.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Styling assumes Tabler defaults. Removing `table-responsive` wrappers was deliberate to prevent dropdown clipping—do not re-add them.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tables/modules.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/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.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 `get_virtual_chassis_member()` for port-to-member mapping and `get_librenms_sync_device()` for VC priority-based device selection in virtual chassis operations
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.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 : Always use exact-only matching (not fuzzy) for site, platform, device type, and role via functions in `utils.py` (`find_matching_site`, `match_librenms_hardware_to_device_type`, `find_matching_platform`)
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.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/templates/**/*.html : Follow frontend conventions for templates and static files defined in `.github/instructions/frontend.instructions.md`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript VLAN modal functions must implement `openVlanDetailModal()`, `verifyVlanInGroup()`, `verifyVlanSyncGroup()` for per-interface VLAN detail editing.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/inc/paginator.html : `inc/paginator.html` is a custom paginator that preserves tab state and `interface_name_field` in pagination URLs. Used across all sync tables.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/tables/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py|**/views/object_sync/**/*.py : Use `LibreNMSAPIMixin` to provide lazy-loaded `LibreNMSAPI` instance via `self.librenms_api` property and `get_server_info()` method for template context.
Applied to files:
netbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/tests/test_integration_sync.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/views/**/*.py : New views must extend the closest base class and compose mixins from `views/mixins.py` (`LibreNMSPermissionMixin`, `NetBoxObjectPermissionMixin`, `LibreNMSAPIMixin`, `CacheMixin`, `VlanAssignmentMixin`)
Applied to files:
netbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.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/views/**/*.py : All views must inherit `LibreNMSPermissionMixin` from `views/mixins.py` with `permission_required = PERM_VIEW_PLUGIN`
Applied to files:
netbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/imports/actions.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 : Reuse `librenms_api.py` client for all LibreNMS communication instead of making direct `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and caching
Applied to files:
netbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : Base view classes (`BaseLibreNMSSyncView`, `BaseInterfaceTableView`, `BaseCableTableView`, `BaseIPAddressTableView`, `BaseVLANTableView`) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results with `CacheMixin` keys like `librenms_{data_type}_{model_name}_{pk}`, compare against NetBox objects, and render a django-tables2 table in a partial template.
Applied to files:
netbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`.
Applied to files:
netbox_librenms_plugin/tests/test_mixins.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Never use `pytest.mark.django_db` for unit tests—mock all database interactions with `MagicMock`.
Applied to files:
netbox_librenms_plugin/tests/test_mixins.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Never use `RequestFactory`—mock request objects directly or test method logic in isolation.
Applied to files:
netbox_librenms_plugin/tests/test_mixins.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py|**/views/object_sync/**/*.py|**/views/sync/**/*.py : Cache keys for sync data must follow the format: `librenms_{data_type}_{model_name}_{pk}` for fetched data and `librenms_{data_type}_last_fetched_{model_name}_{pk}` for fetch timestamps. VLAN group overrides must use `get_vlan_overrides_key(obj)`.
Applied to files:
netbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/views/sync/modules.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/views/sync/**/*.py : Sync POST handlers must call `require_all_permissions()` (not just `require_write_permission()`) and return early if it returns a response; use `require_all_permissions_json()` for AJAX/JSON endpoints
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: All base view classes must follow the three-layer architecture: Base views define abstract data pipelines via `get_*_context()` methods, object sync views wire base views to NetBox models using `register_model_view()`, and sync action views handle POST requests with permissions checks and transactional updates.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : `VlanAssignmentMixin` must resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/__init__.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/object_sync/**/*.py : Object sync view methods must create instances of concrete table views, copy the `request` object, and call `get_context_data()`. VMs must skip cables and VLANs by returning `None` from those `get_*_context()` methods.
Applied to files:
netbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/** : Per-device dropdown update views (`DeviceRoleUpdateView`, `DeviceClusterUpdateView`, `DeviceRackUpdateView`) must apply selection to validation state and return re-rendered row via `render_device_row()`
Applied to files:
netbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Test business logic via the utility modules (import_utils.py, import_validation_helpers.py, etc.) they call, not via HTTP requests, for views in `views/sync/`, `views/object_sync/`, and `views/imports/actions.py`.
Applied to files:
netbox_librenms_plugin/views/__init__.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/tables/modules.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/tables/**/*.py : Table classes in `tables/` must use `ToggleColumn(attrs={'input': {'name': 'select'}})` for selection, accept contextual parameters in constructors (e.g., `device`, `interface_name_field`, `vlan_groups`), set `self.tab` and `self.prefix` for multi-table pagination, include `data-*` attributes in row attrs, and VLAN columns must use `render_vlans()` with hidden inputs and JSON data.
Applied to files:
netbox_librenms_plugin/tables/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript checkbox management must include functions `initializeTableCheckboxes()` and `updateBulkActionButton()` to handle multi-table checkbox selection and bulk action button state.
Applied to files:
netbox_librenms_plugin/tables/modules.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/** : `BulkImportDevicesView` (POST) must execute import: background mode enqueues `ImportDevicesJob`; sync mode calls `bulk_import_devices()` + `bulk_import_vms()` and returns OOB row swaps with `HX-Trigger: closeModal`
Applied to files:
netbox_librenms_plugin/views/imports/actions.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/api/views.py : API endpoints must use `LibreNMSPluginPermission` class in `api/views.py` where GET checks `view_librenmssettings` and other methods check `change_librenmssettings`
Applied to files:
netbox_librenms_plugin/views/imports/actions.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/jobs.py : Follow background job conventions defined in `.github/instructions/background-jobs.instructions.md` for `jobs.py`, import views, and import utilities
Applied to files:
netbox_librenms_plugin/views/imports/actions.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/jobs.py : Background jobs must use standalone helpers from `import_utils.py` (`check_user_permissions`, `require_permissions`) instead of view mixins; non-superusers fall back to synchronous mode
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_utils.py : `validate_device_for_import(device, ...)` is the core validation function that produces validation state dict
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/** : `DeviceImportHelperMixin` must provide `get_validated_device_with_selections()` and `render_device_row()` for HTMX row rendering, shared by update views
Applied to files:
netbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/urls.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_validation_helpers.py : Validation helpers must include: `fetch_model_by_id()`, `extract_device_selections()` for reading form data
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
Comment on lines
+38
to
+40
| class Meta: | ||
| attrs = {"class": "table table-hover object-list", "id": "librenms-module-table"} | ||
| row_attrs = {"class": lambda record: record.get("row_class", "")} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add row-level data-* attributes to Meta.row_attrs.
Line 40 currently sets only a CSS class. This table contract expects row data-* attributes for consistent sync-table behavior.
♻️ Proposed fix
class Meta:
attrs = {"class": "table table-hover object-list", "id": "librenms-module-table"}
- row_attrs = {"class": lambda record: record.get("row_class", "")}
+ row_attrs = {
+ "class": lambda record: record.get("row_class", ""),
+ "data-ent-physical-index": lambda record: record.get("ent_physical_index", ""),
+ "data-status": lambda record: record.get("status", ""),
+ }As per coding guidelines: “Table classes in tables/ must … include data-* attributes in row attrs.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tables/modules.py` around lines 38 - 40,
Meta.row_attrs currently only sets the CSS class; add the required row-level
data-* attributes here so sync-table behavior can read them. Update the Meta
class's row_attrs (the lambda currently referenced by row_attrs) to return a
dict that includes the existing "class" plus the needed data-* keys (e.g.,
"data-id", "data-type" or whatever the sync-table contract expects) populated
from the record via record.get(...); ensure the lambda still falls back to empty
strings when keys are missing.
Comment on lines
+134
to
+142
| if warning := record.get("module_path_warning"): | ||
| return format_html( | ||
| '<span class="badge {}" title="{}">{}</span>' | ||
| ' <i class="mdi mdi-alert-outline text-warning" title="{}"></i>', | ||
| badge_class, | ||
| warning, | ||
| value, | ||
| "Upgrade NetBox to fully support {module_path}", | ||
| ) |
There was a problem hiding this comment.
Fix the literal {module_path} tooltip placeholder.
Line 141 renders {module_path} literally, so the user-facing tooltip is incorrect.
🩹 Proposed fix
if warning := record.get("module_path_warning"):
+ module_path = record.get("module_path", "module path")
return format_html(
'<span class="badge {}" title="{}">{}</span>'
' <i class="mdi mdi-alert-outline text-warning" title="{}"></i>',
badge_class,
warning,
value,
- "Upgrade NetBox to fully support {module_path}",
+ f"Upgrade NetBox to fully support {module_path}",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tables/modules.py` around lines 134 - 142, The tooltip
string is currently a literal "{module_path}" and never interpolated; update the
format_html call in modules.py (the block that checks
record.get("module_path_warning")) to use a format placeholder and pass the
actual module path value (e.g., record.get("module_path") or module_path) as an
additional argument—for example change the final argument to a format string
like "Upgrade NetBox to fully support {}" and supply the module path so the
tooltip shows the real module path instead of the literal placeholder.
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 @.github/pull_request_template.md:
- Around line 1-49: The Markdown headings (e.g., "## Summary", "## Motivation /
Problem", "## Scope of Change", "## How Was This Tested?", "## Risk Assessment",
"## Backwards Compatibility", "## Other Notes") lack the required blank lines
above and/or below them; update the pull request template so each heading has a
blank line before and after the heading (insert a single empty line between
adjacent headings and between headings and paragraph content) to satisfy MD022
and ensure consistent rendering across Markdown parsers.
In `@pyproject.toml`:
- Around line 52-55: Clarify and resolve the contradictory comments about C901
in the [tool.ruff.lint.mccabe] block: either enable the complexity rule by
adding an explicit selector (e.g., extend-select = ["C901"]) so max-complexity =
15 takes effect, and update the comment to state that C901 will be enforced at
15, or leave C901 disabled and change the comment to clearly state that
max-complexity is recorded for manual review only and C901 is not selected;
update the lines around the tool.ruff.lint.mccabe section and the comment above
max-complexity to reflect the chosen behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b36c2ea4-1bc3-4e9a-87db-e4697e15037f
📒 Files selected for processing (4)
.github/dependabot.yml.github/pull_request_template.md.pre-commit-config.yamlpyproject.toml
📜 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.14)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.12)
🧰 Additional context used
🪛 markdownlint-cli2 (0.21.0)
.github/pull_request_template.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 4-4: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 38-38: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 48-48: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (3)
.github/dependabot.yml (1)
11-14:uvDependabot entry is correctly scoped and consistent.Good addition. The new weekly root-level
uvupdate block is valid and complements the existinggithub-actionsupdates without increasing config complexity..pre-commit-config.yaml (2)
3-3: Good hook revision pinning for reproducibility.Pinning
ruff-pre-committo a specific tag keeps hook behavior deterministic across environments.
17-17: Targeted YAML exclusion looks correct.This exclusion is precise and prevents
check-yamlfrom skipping unrelated YAML files.
- modules_view: guard against KeyError on missing entPhysicalIndex in index_map builds
- modules_view: replace fixed range(10) ancestor traversal loop with visited-set while loop
- modules_view: preload ModuleBayMapping in _build_context to eliminate N+1 queries in _match_module_bay
- modules_view: update _lookup_regex_bay_mapping signature to accept preloaded list instead of querying DB
- sync/modules: use require_all_permissions (HTML redirect) instead of JSON variant for form-based POST handlers
- sync/modules: use ordered unique list (dict.fromkeys) instead of set for selected_indices to preserve install order
- sync/modules: guard index_map build in InstallSelectedView against missing entPhysicalIndex
- _module_sync_content.html: fix nested forms; move Install Selected into standalone form above table
- librenms_sync.js: add initializeInstallSelectedForm() to collect checked rows before form submit
- bulk_import: filter placeholder serials ('-', whitespace) from vc_domain member_serials
- mock_librenms_server: add server_close() and thread join to stop() for proper cleanup
- test_librenms_id: assert filter Q covers both JSON server-key and legacy integer paths
- vm_operations: validate/convert device_id before VirtualMachine.objects.create
- test_sync_modules: always import modules_view in test_install_module_view_not_in_base
Bug 1: ToggleColumn in LibreNMSModuleTable lacked an accessor, so the resolved cell value was always '' (empty_values) and render() was never called → no per-row checkboxes. Fix: add accessor='ent_physical_index' which is present on every row dict produced by _build_row. Bug 2: On Cisco 8201-style inventory the top-level chassis module (idx 1) is wrapped in a container that has model='N/A'. The ancestor-walk skipped containers only when their model was truly empty (not anc_model), but 'N/A' is truthy, so the container was treated as a real ancestor and the chassis module was erroneously marked is_descendant=True → excluded from top_items → no rows and no transceivers. Fix: extend the skip condition to check anc_model in _GENERIC_CONTAINER_MODELS instead of not anc_model. Add regression tests for both bugs: - TestToggleColumnAccessor: asserts selection column has correct accessor and that a record with ent_physical_index produces a non-empty checkbox. - TestAncestorWalkGenericContainerModel: asserts items under N/A containers appear as top-level items, and that real parent/child relationships are still handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
netbox_librenms_plugin/views/sync/modules.py (1)
102-103:⚠️ Potential issue | 🟠 MajorGuard
entPhysicalIndexreads from cached LibreNMS inventory rows.Line 102, Line 159, and Line 173 index
entPhysicalIndexdirectly. A malformed row will raiseKeyErrorand fail the whole install flow.♻️ Proposed fix
- index_map = {item["entPhysicalIndex"]: item for item in cached_data} + index_map = {idx: item for item in cached_data if (idx := item.get("entPhysicalIndex")) is not None} @@ - parent = next((i for i in inventory_data if i["entPhysicalIndex"] == parent_index), None) + parent = next((i for i in inventory_data if i.get("entPhysicalIndex") == parent_index), None) @@ - child_idx = child["entPhysicalIndex"] + child_idx = child.get("entPhysicalIndex") + if child_idx is None: + continueAlso applies to: 159-160, 173-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/modules.py` around lines 102 - 103, The code assumes every cached LibreNMS row has "entPhysicalIndex" and directly indexes it (e.g., when building index_map = {item["entPhysicalIndex"]: item for item in cached_data} and later accesses item["entPhysicalIndex"] in _collect_branch/related iteration), which can raise KeyError on malformed rows; update these spots to safely read the key (use item.get("entPhysicalIndex") or check "entPhysicalIndex" in item) and skip or log rows missing the field so they are excluded from index_map and from branch traversal (refer to index_map, cached_data, _collect_branch, branch_items to locate all occurrences).netbox_librenms_plugin/import_utils/bulk_import.py (1)
527-529:⚠️ Potential issue | 🟠 MajorNormalize DB job status before cancellation checks.
Line 528 and Line 557 compare
job.job.statusdirectly. If it’s enum-like, cancellation can be missed and processing continues unexpectedly.♻️ Proposed fix
except Exception: # Fall back to DB check if RQ check fails job.job.refresh_from_db() - if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED): + status = job.job.status + status_value = status.value if hasattr(status, "value") else status + if status_value in ( + JobStatusChoices.STATUS_FAILED, + JobStatusChoices.STATUS_ERRORED, + "failed", + "errored", + ): job.logger.warning("Job was stopped before validation started") return _empty_return(return_cache_status) @@ except Exception: # If we can't check RQ status, fall back to DB status check job.job.refresh_from_db() - if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED): + status = job.job.status + status_value = status.value if hasattr(status, "value") else status + if status_value in ( + JobStatusChoices.STATUS_FAILED, + JobStatusChoices.STATUS_ERRORED, + "failed", + "errored", + ): job.logger.info(f"Job stopped at device {idx}/{total}. Exiting gracefully.") return _empty_return(return_cache_status)Based on learnings: Applies to /views/imports/,**/import_utils.py : Recognize database Job status values as
pending,scheduled,running,completed,failed,errored(NOcancelledstatus exists).Also applies to: 557-559
🤖 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 527 - 529, Normalize the DB job status before performing cancellation/stop checks by converting job.job.status to a canonical string or using the JobStatusChoices constants for comparison; replace direct enum-like comparisons at job.job.status (the checks that currently use JobStatusChoices.STATUS_FAILED and JobStatusChoices.STATUS_ERRORED around the refresh block and the later check around lines 557-559) with a normalized value (e.g., str(job.job.status).lower() or job.job.status.value) and compare against the known status names ('failed', 'errored', 'pending', 'scheduled', 'running', 'completed') so cancelled-like states are not missed and the same normalization is applied to both the early stop warning and the later cancellation logic.netbox_librenms_plugin/views/base/modules_view.py (2)
601-612:⚠️ Potential issue | 🟡 MinorReplace fixed ancestor hop limit with visited-set traversal.
Line 601 caps traversal at 5 levels. Deep-but-valid container chains will fail to match bays even without cycles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 601 - 612, The traversal currently limited by "for _ in range(5)" can skip deep-but-valid containment chains; change it to a loop that walks up using current_idx and index_map until a model is found or traversal terminates, using a visited set to detect cycles and avoid infinite loops. Specifically, replace the fixed-iteration loop around current_idx/container_idx/ancestor with a while current_idx and current_idx in index_map loop, record visited indices (e.g., visited = set()) and break/return None on revisiting, update container_idx each step, inspect ancestor.get("entPhysicalModelName") like before, and return None if you exit the loop without finding a model; keep references to index_map, current_idx, container_idx, ancestor and entPhysicalContainedIn to locate the code.
201-201:⚠️ Potential issue | 🟠 MajorStill vulnerable to
KeyErroron malformed inventory rows.Line 201, Line 286, and Line 386 directly index
entPhysicalIndex. These are external payloads and should be guarded consistently.♻️ Proposed fix
- sub_items = self._get_sub_components(item["entPhysicalIndex"], inventory_data) + parent_idx = item.get("entPhysicalIndex") + if parent_idx is None: + continue + sub_items = self._get_sub_components(parent_idx, inventory_data) @@ - inv_by_index = {item["entPhysicalIndex"]: item for item in inventory_data} + inv_by_index = { + idx: item for item in inventory_data if (idx := item.get("entPhysicalIndex")) is not None + } @@ - child_idx = child["entPhysicalIndex"] + child_idx = child.get("entPhysicalIndex") + if child_idx is None: + continueAlso applies to: 286-286, 386-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` at line 201, Several places in modules_view.py directly index item["entPhysicalIndex"] (e.g., the call to _get_sub_components at line with sub_items = self._get_sub_components(item["entPhysicalIndex"], inventory_data)) and thus can raise KeyError for malformed inventory rows; update those sites (and the similar occurrences referenced at the other two locations) to safely access the key (for example use item.get("entPhysicalIndex") and skip/continue when it's None or missing, or wrap the access in a try/except KeyError that logs and skips the row) so _get_sub_components and any downstream logic never receive a missing index. Ensure you apply the same guard at every place that currently does direct indexing of "entPhysicalIndex".netbox_librenms_plugin/tests/test_sync_modules.py (1)
808-843: 🧹 Nitpick | 🔵 TrivialThis test helper duplicates production logic instead of validating it.
_run_top_items()mirrors the algorithm fromBaseModuleTableView; regressions in the real implementation can slip through. Prefer invoking the view logic (or a shared extracted helper) directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_sync_modules.py` around lines 808 - 843, The helper _run_top_items reproduces production logic (mirroring BaseModuleTableView) instead of exercising it; replace this duplication by invoking the real logic: call BaseModuleTableView's method that computes top items (or extract the shared algorithm into a single helper used by both the view and tests) and have the test use that function; ensure the test imports INVENTORY_CLASSES and _GENERIC_CONTAINER_MODELS only if needed for expected outputs, otherwise remove duplicate checks and assert against the view/helper's output to catch regressions.netbox_librenms_plugin/tables/modules.py (2)
135-143:⚠️ Potential issue | 🟡 MinorTooltip text currently shows a literal placeholder.
Line 142 renders
{module_path}literally, which is misleading in UI.♻️ Proposed fix
- "Upgrade NetBox to fully support {module_path}", + "Upgrade NetBox to fully support module_path templates",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/modules.py` around lines 135 - 143, The tooltip string currently contains the literal "{module_path}"; update the format_html call in the block handling record.get("module_path_warning") to include the actual module path value instead of the placeholder by using a positional placeholder and passing the module path (e.g., record.get("module_path") or the existing value variable) as the last argument; adjust the title argument so it becomes "Upgrade NetBox to fully support {}" and ensure the final argument is the module path value so format_html interpolates it correctly (refer to module_path_warning, format_html, badge_class, and value).
41-41: 🛠️ Refactor suggestion | 🟠 MajorAdd required row-level
data-*attributes inMeta.row_attrs.Only class is set today. Sync table behavior depends on row metadata as part of the table contract.
♻️ Proposed fix
- row_attrs = {"class": lambda record: record.get("row_class", "")} + row_attrs = { + "class": lambda record: record.get("row_class", ""), + "data-ent-physical-index": lambda record: record.get("ent_physical_index", ""), + "data-status": lambda record: record.get("status", ""), + }As per coding guidelines: “Table classes in
tables/must … includedata-*attributes in row attrs.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/modules.py` at line 41, Meta.row_attrs currently only sets the row CSS class; extend the row_attrs dict in the Table class (the variable row_attrs in netbox_librenms_plugin/tables/modules.py) to include the required row-level data-* attributes used by the sync contract (e.g., add keys like "data-id", "data-pk", "data-sync" mapped to lambdas that return record.get("id", ""), record.get("pk", "") and record.get("sync", "") respectively), so each row emits the expected data-* metadata from the record; ensure the keys match the sync contract and use record.get(..., "") to avoid KeyError.
🤖 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/bulk_import.py`:
- Around line 345-359: In _refresh_existing_device(), stop manually recomputing
can_import and is_ready from validation and instead delegate state mutation to
the centralized helpers in import_validation_helpers.py: remove the inline logic
that sets validation["can_import"] and validation["is_ready"] (and the
intermediate can_import/is_ready/local checks) and call the appropriate
helper(s) used elsewhere in the pipeline (the functions that recalculate import
readiness and apply role/cluster/rack assignment and issue removal) so the
validation dict (keys like "issues", "import_as_vm", "cluster", "site",
"device_type", "device_role", "can_import", "is_ready") is mutated consistently
by the shared utilities rather than duplicated here.
In `@netbox_librenms_plugin/import_utils/vm_operations.py`:
- Around line 62-74: Wrap the VM creation and the LibreNMS ID assignment in a
single atomic DB transaction so they succeed or fail together: perform
VirtualMachine.objects.create(...) and the subsequent set_librenms_device_id(vm,
librenms_device_id, server_key) call inside a django.db.transaction.atomic()
block, or alternatively pass the custom_field_data/librenms_id into the create
call so the object is created with the mapping in one write; ensure only a
single vm.save() (or none if created with the field) is performed and remove the
separate post-create write to avoid leaving an unmapped VM on partial failure.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1412-1417: initializeInstallSelectedForm currently adds a new
submit listener every time initializeScripts() runs (e.g., after HTMX swaps),
causing duplicate handlers; make it idempotent by attaching a single named
handler or guarding with a flag/data attribute: define the submit handler as a
separate function (e.g., handleInstallSelectedSubmit) and before adding call
form.removeEventListener('submit', handleInstallSelectedSubmit) then
form.addEventListener('submit', handleInstallSelectedSubmit) or set/check
form.dataset.installInit = "true" and skip re-adding if already initialized;
ensure this change is applied inside initializeInstallSelectedForm and that
initializeScripts() continues to call it on DOMContentLoaded and htmx:afterSwap.
In `@netbox_librenms_plugin/tests/mock_librenms_server.py`:
- Around line 108-122: The default mock in ports_response is missing standard
port fields requested by librenms_api.get_ports; update the default ports dict
returned by ports_response (method ports_response) to include the missing keys
such as ifMtu, ifVlan, and ifTrunk (use sensible defaults like an integer MTU,
integer or null VLAN, and boolean/0 for trunk) so tests that read
ifMtu/ifVlan/ifTrunk from the mocked /api/v0/devices/{device_id}/ports response
match the real API shape.
- Around line 70-73: The stop method may return before the server thread
actually exits; after calling self._thread.join(timeout=5) in stop(), check
self._thread.is_alive() and handle the case where it remains alive (e.g., log a
warning including thread identity and consider taking additional cleanup like
calling self._server.shutdown()/server_close() again or setting a flag to
prevent socket reuse in tests). Update the stop() implementation (referencing
stop, self._server.shutdown, self._server.server_close, and
self._thread.join/is_alive) to detect a non-joined thread and emit a clear
warning so tests know the shutdown did not complete cleanly.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 357-360: The call to BaseModuleTableView._lookup_regex_bay_mapping
is passing the ModuleBayMapping class instead of the iterable of regex mappings;
change the call in the loop over candidate_names to pass the module_bays
iterable (module_bays) rather than ModuleBayMapping so _lookup_regex_bay_mapping
receives the expected iterable of mapping objects (retain re, name, phys_class
arguments as-is).
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 527-529: Normalize the DB job status before performing
cancellation/stop checks by converting job.job.status to a canonical string or
using the JobStatusChoices constants for comparison; replace direct enum-like
comparisons at job.job.status (the checks that currently use
JobStatusChoices.STATUS_FAILED and JobStatusChoices.STATUS_ERRORED around the
refresh block and the later check around lines 557-559) with a normalized value
(e.g., str(job.job.status).lower() or job.job.status.value) and compare against
the known status names ('failed', 'errored', 'pending', 'scheduled', 'running',
'completed') so cancelled-like states are not missed and the same normalization
is applied to both the early stop warning and the later cancellation logic.
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 135-143: The tooltip string currently contains the literal
"{module_path}"; update the format_html call in the block handling
record.get("module_path_warning") to include the actual module path value
instead of the placeholder by using a positional placeholder and passing the
module path (e.g., record.get("module_path") or the existing value variable) as
the last argument; adjust the title argument so it becomes "Upgrade NetBox to
fully support {}" and ensure the final argument is the module path value so
format_html interpolates it correctly (refer to module_path_warning,
format_html, badge_class, and value).
- Line 41: Meta.row_attrs currently only sets the row CSS class; extend the
row_attrs dict in the Table class (the variable row_attrs in
netbox_librenms_plugin/tables/modules.py) to include the required row-level
data-* attributes used by the sync contract (e.g., add keys like "data-id",
"data-pk", "data-sync" mapped to lambdas that return record.get("id", ""),
record.get("pk", "") and record.get("sync", "") respectively), so each row emits
the expected data-* metadata from the record; ensure the keys match the sync
contract and use record.get(..., "") to avoid KeyError.
In `@netbox_librenms_plugin/tests/test_sync_modules.py`:
- Around line 808-843: The helper _run_top_items reproduces production logic
(mirroring BaseModuleTableView) instead of exercising it; replace this
duplication by invoking the real logic: call BaseModuleTableView's method that
computes top items (or extract the shared algorithm into a single helper used by
both the view and tests) and have the test use that function; ensure the test
imports INVENTORY_CLASSES and _GENERIC_CONTAINER_MODELS only if needed for
expected outputs, otherwise remove duplicate checks and assert against the
view/helper's output to catch regressions.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 601-612: The traversal currently limited by "for _ in range(5)"
can skip deep-but-valid containment chains; change it to a loop that walks up
using current_idx and index_map until a model is found or traversal terminates,
using a visited set to detect cycles and avoid infinite loops. Specifically,
replace the fixed-iteration loop around current_idx/container_idx/ancestor with
a while current_idx and current_idx in index_map loop, record visited indices
(e.g., visited = set()) and break/return None on revisiting, update
container_idx each step, inspect ancestor.get("entPhysicalModelName") like
before, and return None if you exit the loop without finding a model; keep
references to index_map, current_idx, container_idx, ancestor and
entPhysicalContainedIn to locate the code.
- Line 201: Several places in modules_view.py directly index
item["entPhysicalIndex"] (e.g., the call to _get_sub_components at line with
sub_items = self._get_sub_components(item["entPhysicalIndex"], inventory_data))
and thus can raise KeyError for malformed inventory rows; update those sites
(and the similar occurrences referenced at the other two locations) to safely
access the key (for example use item.get("entPhysicalIndex") and skip/continue
when it's None or missing, or wrap the access in a try/except KeyError that logs
and skips the row) so _get_sub_components and any downstream logic never receive
a missing index. Ensure you apply the same guard at every place that currently
does direct indexing of "entPhysicalIndex".
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 102-103: The code assumes every cached LibreNMS row has
"entPhysicalIndex" and directly indexes it (e.g., when building index_map =
{item["entPhysicalIndex"]: item for item in cached_data} and later accesses
item["entPhysicalIndex"] in _collect_branch/related iteration), which can raise
KeyError on malformed rows; update these spots to safely read the key (use
item.get("entPhysicalIndex") or check "entPhysicalIndex" in item) and skip or
log rows missing the field so they are excluded from index_map and from branch
traversal (refer to index_map, cached_data, _collect_branch, branch_items to
locate all occurrences).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1aebdefc-7261-4181-a975-289908957b3a
📒 Files selected for processing (10)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/modules.py
sync/modules.py: _match_bay passed ModuleBayMapping class to _lookup_regex_bay_mapping instead of the preloaded is_regex=True list; fix by pre-fetching regex mappings before the loop and passing the list. Also guard InstallBranchView index_map build against missing entPhysicalIndex (mirrors the existing guard in InstallSelectedView). modules_view.py: replace fixed-bound 'for _ in range(5)' loop in _match_bay_by_position with a visited-set while loop so deeper containment chains are handled correctly and cycles are detected. Also guard 'item["entPhysicalIndex"]' direct access with .get() + continue so malformed top_items rows are skipped safely. vm_operations.py: wrap VirtualMachine.objects.create and set_librenms_device_id in a single transaction.atomic() block so a failure during ID assignment never leaves a VM without a LibreNMS mapping. Update test fixture with an autouse _patch_atomic to keep tests DB-free. librenms_sync.js: initializeInstallSelectedForm previously added a new submit listener on every HTMX swap. Guard with form.dataset.installInit flag so the listener is registered exactly once per form element. mock_librenms_server.py: add thread.is_alive() warning after join() so test failures from unreleased sockets are surfaced immediately. Also add ifMtu/ifVlan/ifTrunk defaults to ports_response to match the real LibreNMS /ports API shape.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
netbox_librenms_plugin/views/base/modules_view.py (1)
289-289:⚠️ Potential issue | 🟠 MajorGuard
entPhysicalIndexreads from external payloads to avoid request-fatalKeyError.Line 289 and Line 389 assume
entPhysicalIndexalways exists in LibreNMS data. A malformed row will abort module sync.💡 Suggested hardening
- inv_by_index = {item["entPhysicalIndex"]: item for item in inventory_data} + inv_by_index = { + idx: item for item in inventory_data if (idx := item.get("entPhysicalIndex")) is not None + } @@ - child_idx = child["entPhysicalIndex"] + child_idx = child.get("entPhysicalIndex") + if child_idx is None: + continueAlso applies to: 389-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` at line 289, The code builds inv_by_index = {item["entPhysicalIndex"]: item for item in inventory_data} without guarding missing keys which can raise KeyError on malformed LibreNMS rows; update the comprehension to skip or fallback when "entPhysicalIndex" is missing (e.g., use item.get("entPhysicalIndex") and filter out None) so malformed rows don't abort sync, and apply the same defensive change where "entPhysicalIndex" is read later around the logic that references it (the usage at/near lines 389-390); ensure any downstream lookups handle absent keys gracefully.
🤖 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/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Line 1417: The forEach callback used on
form.querySelectorAll('input[data-injected-select]') uses an expression-bodied
arrow (el => el.remove()) which triggers the Biome lint rule; change it to a
block-bodied callback (e.g., el => { el.remove(); }) so the callback has an
explicit block body. Update the call site where
form.querySelectorAll(...).forEach(...) is invoked to use the block-bodied arrow
to satisfy lint/suspicious/useIterableCallbackReturn and avoid CI failures.
In `@netbox_librenms_plugin/tests/test_vm_operations.py`:
- Around line 30-41: Replace the ad-hoc inline test data and MagicMock objects
(e.g., libre_device, mock_cluster, mock_platform, validation) with the shared
fixtures from tests/conftest.py: use mock_librenms_api, sample_librenms_device
(instead of libre_device), sample_validation_state_vm (instead of the validation
dict), and the NetBox object fixtures provided there; update test functions in
test_vm_operations.py (including the other occurrences around the previously
noted blocks) to accept these fixtures as parameters and remove the manual
MagicMock constructions so the tests rely on the centralized fixture instances.
- Around line 291-315: The test should assert that the server_key from the input
is forwarded to VM creation: when calling bulk_import_vms({20: {"server_key":
"expected-key"}}, ...) verify the patched create_vm_from_librenms was called
with server_key="expected-key" (use the mock returned by patch of
create_vm_from_librenms to inspect its call args). Update the existing
with-patch block around bulk_import_vms in test_vm_operations.py to supply a
server_key in the input and add an assertion referencing
create_vm_from_librenms.assert_called_once_with or checking its call_args for
the server_key parameter; keep other patches (fetch_device_with_cache,
validate_device_for_import, _determine_device_name, Cluster, DeviceRole,
apply_cluster_to_validation, apply_role_to_validation) unchanged.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 173-175: The bug arises because the variable parent_idx is used
both as the table row index and later overwritten with the inventory
entPhysicalIndex, leading to wrong indexing/IndexError when accessing
table_data; change to two distinct variables (e.g., parent_row_idx for the
append/index into table_data and parent_ent_physical_index for the
entPhysicalIndex value) and update all references in this block (the code that
appends to table_data, the code that assigns parent_idx = entPhysicalIndex, and
subsequent reads/writes that use table_data[parent_idx]) so that table_data is
always indexed with the row index variable (parent_row_idx) while any inventory
entPhysicalIndex uses the separate variable (parent_ent_physical_index).
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 345-360: The _match_bay function currently executes
ModuleBayMapping ORM queries per candidate which causes N+1 query amplification;
preload both exact (is_regex=False) and regex (is_regex=True) ModuleBayMapping
sets once in the POST handler or per-batch, then change _install_single and
_match_bay signatures to accept these preloaded mappings (e.g., exact_mappings,
regex_mappings), replace the inline ModuleBayMapping.objects.filter(...) calls
in _match_bay with lookups against exact_mappings (matching librenms_name and
librenms_class/empty) and pass regex_mappings into
BaseModuleTableView._lookup_regex_bay_mapping instead of refetching; update all
callers of _install_single/_match_bay accordingly so no per-item ORM queries
remain.
- Around line 153-181: The code currently assumes every inventory row has
entPhysicalIndex and accesses it directly in _collect_branch and
_collect_children, which can raise KeyError; update both functions to guard
those accesses by using dict.get("entPhysicalIndex") (or "in" checks) and skip
any rows missing entPhysicalIndex, similarly use .get("entPhysicalContainedIn")
when filtering children and .get("entPhysicalModelName") safely as already done;
ensure parent is located only when parent_index matches a valid integer key, and
avoid adding or recursing on rows without a valid entPhysicalIndex so the
install flow won't crash on malformed inventory rows.
- Around line 330-337: The parent-name resolution in _match_bay (using
contained_in, parent_name, index_map) only checks the immediate parent and
should instead traverse the containment hierarchy like the logic in
netbox_librenms_plugin/views/base/modules_view.py; update the block that
computes parent_name to walk up index_map via entPhysicalContainedIn repeatedly
until a container with an entPhysicalName is found (or until root), and use that
resolved name for bay matching in _match_bay so nested items get the same parent
container resolution as the base flow.
- Around line 296-321: Replace the fixed-depth traversal that uses "for _ in
range(10)" with a cycle-safe visited walk: create a visited set and loop until
you either find a matching bay or detect a already-seen parent index (cycle) or
hit a missing parent, using the same variables and checks (current, parent_idx
from entPhysicalContainedIn, index_map, parent_name/parent_descr from
entPhysicalName/entPhysicalDescr, device_bays, mapping_by_name, bay_by_name, and
bay.installed_module.pk) to perform bay matching; when you encounter a
parent_idx that's falsy or not in index_map return None, and if parent_idx is in
visited stop and return None to avoid infinite loops.
---
Duplicate comments:
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Line 289: The code builds inv_by_index = {item["entPhysicalIndex"]: item for
item in inventory_data} without guarding missing keys which can raise KeyError
on malformed LibreNMS rows; update the comprehension to skip or fallback when
"entPhysicalIndex" is missing (e.g., use item.get("entPhysicalIndex") and filter
out None) so malformed rows don't abort sync, and apply the same defensive
change where "entPhysicalIndex" is read later around the logic that references
it (the usage at/near lines 389-390); ensure any downstream lookups handle
absent keys gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbfb64da-28a8-4bd3-9f68-9d3824d73f48
📒 Files selected for processing (6)
netbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/modules.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.14)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.13)
🧰 Additional context used
📓 Path-based instructions (8)
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/tests/mock_librenms_server.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tests/test_vm_operations.py
netbox_librenms_plugin/{templates,static}/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/{templates,static}/**/*.{html,js}: All HTMX requests andfetch()calls must include a CSRF token. The standard pattern isdocument.querySelector('[name=csrfmiddlewaretoken]').value(from a hidden form input). The import JS also usesgetCookie('csrftoken')as a fallback — prefer the hidden input approach for consistency.
Modals use Tabler (Bootstrap-like) but withoutbootstrap.Modalhelpers. Buttons target thehtmx-modal-contentelement and JavaScript inlibrenms_import.htmltoggles the wrapper. Do not reintroducedata-bs-toggleor duplicate modal IDs.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
netbox_librenms_plugin/static/**/*.js
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/static/**/*.js: Always checkresponse.okbefore processing fetch responses to catch HTTP errors. In catch blocks, showerror.messagefor debugging rather than generic messages.
ThecreateCacheCountdown()function is a generic countdown timer for cache expiration display.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
**/librenms_sync.js
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/librenms_sync.js: JavaScript inlibrenms_sync.jsmust not be wrapped in an IIFE and must use a master initializerinitializeScripts()that runs on bothDOMContentLoadedandhtmx:afterSwapevents.
JavaScript checkbox management must include functionsinitializeTableCheckboxes()andupdateBulkActionButton()to handle multi-table checkbox selection and bulk action button state.
JavaScript TomSelect dropdown initialization must use aTOMSELECT_INIT_DELAY_MS = 100constant and implement delayed initialization after HTMX swaps. Required initializer functions:initializeVCMemberSelect(),initializeVRFSelects(),initializeVlanGroupSelects(),initializeVlanSyncGroupSelects().
JavaScript verification functions must includehandleInterfaceChange(),handleCableChange(),handleVRFChange()that POST to single-item verify endpoints to validate resource changes.
JavaScript VLAN modal functions must implementopenVlanDetailModal(),verifyVlanInGroup(),verifyVlanSyncGroup()for per-interface VLAN detail editing.
JavaScript bulk operations must include functionsinitializeBulkEditApply()anddeleteSelectedInterfaces()to handle bulk edit and delete actions.
JavaScript table filtering must implementinitializeTableFilters()andfilterTable()functions for client-side row filtering.
JavaScript URL and tab state management must implementinitializeTabs(),getDeviceIdFromUrl(), andsetInterfaceNameFieldFromURL()to maintain browser state and URL synchronization.
JavaScript cache countdown functionality must implementinitializeCountdown()andinitializeCountdowns()functions to display and manage cache expiration timers.
JavaScript CSRF token must be extracted viadocument.querySelector('[name=csrfmiddlewaretoken]').valuefor all POST requests.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
netbox_librenms_plugin/views/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/views/**/*.py: Views must follow a three-layer structure: Base views inviews/base/, Object sync views inviews/object_sync/, and Sync action views inviews/sync/with shared mixins fromviews/mixins.py
New views must extend the closest base class and compose mixins fromviews/mixins.py(LibreNMSPermissionMixin,NetBoxObjectPermissionMixin,LibreNMSAPIMixin,CacheMixin,VlanAssignmentMixin)
All views must inheritLibreNMSPermissionMixinfromviews/mixins.pywithpermission_required = PERM_VIEW_PLUGIN
Declarerequired_object_permissionsas a dict mapping HTTP methods to[(action, Model)]tuples for NetBox model operations; some views may set this dynamically per-request
Use_get_safe_redirect_url(request)to validate referrer URLs in permission checks to prevent open-redirect attacks
Files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
netbox_librenms_plugin/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/views/sync/**/*.py: Sync POST handlers must callrequire_all_permissions()(not justrequire_write_permission()) and return early if it returns a response; userequire_all_permissions_json()for AJAX/JSON endpoints
Follow sync conventions defined in.github/instructions/sync.instructions.mdfor sync views, base views, tables, and sync JavaScript
Files:
netbox_librenms_plugin/views/sync/modules.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/modules.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/modules_view.py
🧠 Learnings (54)
📓 Common learnings
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/api/**/*.py : Update API serializers in `api/serializers.py` and `api/views.py` together to prevent contract drift between models and external API consumers
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Reuse fixtures from `tests/conftest.py` instead of creating ad-hoc mocks: Configuration (`mock_multi_server_config`, `mock_legacy_config`), API client (`mock_librenms_api`), NetBox objects (`mock_netbox_device`, `mock_netbox_vm`, `mock_netbox_site`, `mock_netbox_platform`, `mock_netbox_device_type`, `mock_netbox_device_role`, `mock_netbox_cluster`, `mock_netbox_rack`), HTTP responses (`mock_response_factory`, `mock_success_response`, `mock_device_response`, `mock_error_response`, `mock_auth_error_response`), and Import workflow (`sample_librenms_device`, `sample_librenms_device_minimal`, `sample_validation_state`, `sample_validation_state_vm`).
Applied to files:
netbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/test_netbox_librenms_plugin.py : `test_netbox_librenms_plugin.py` is an empty placeholder — do not add tests there.
Applied to files:
netbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances.
Applied to files:
netbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_vm_operations.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 : Reuse `librenms_api.py` client for all LibreNMS communication instead of making direct `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and caching
Applied to files:
netbox_librenms_plugin/tests/mock_librenms_server.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py|**/views/object_sync/**/*.py : Use `LibreNMSAPIMixin` to provide lazy-loaded `LibreNMSAPI` instance via `self.librenms_api` property and `get_server_info()` method for template context.
Applied to files:
netbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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: Keep REST endpoint server responses and HTMX targets in sync; implement REST endpoints in `views/imports/actions.py` and HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript checkbox management must include functions `initializeTableCheckboxes()` and `updateBulkActionButton()` to handle multi-table checkbox selection and bulk action button state.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/static/**/*import*.js : The `initializeFilterForm()` function intercepts form submit, detects JSON response (background job), and starts polling.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript bulk operations must include functions `initializeBulkEditApply()` and `deleteSelectedInterfaces()` to handle bulk edit and delete actions.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/static/**/*import*.js : The `captureSelectionState()` and `restoreSelectionState()` functions preserve checkbox state across HTMX content swaps.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript TomSelect dropdown initialization must use a `TOMSELECT_INIT_DELAY_MS = 100` constant and implement delayed initialization after HTMX swaps. Required initializer functions: `initializeVCMemberSelect()`, `initializeVRFSelects()`, `initializeVlanGroupSelects()`, `initializeVlanSyncGroupSelects()`.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/static/**/*import*.js : Import page JavaScript (`librenms_import.js`) is wrapped in an IIFE with `window.LibreNMSImportInitialized` guard to prevent re-initialization during HTMX swaps.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript verification functions must include `handleInterfaceChange()`, `handleCableChange()`, `handleVRFChange()` that POST to single-item verify endpoints to validate resource changes.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/static/**/*import*.js : The import page uses `ModalManager` class and `filterModalManager` instance—always use this reference in fetch callbacks, not undefined `modalInstance` variables.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/{templates,static}/**/*.{html,js} : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers. Buttons target the `htmx-modal-content` element and JavaScript in `librenms_import.html` toggles the wrapper. Do not reintroduce `data-bs-toggle` or duplicate modal IDs.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/htmx/*import*.html : Device import dropdowns rely on TomSelect decorators set up elsewhere. Keep `<select class="device-role-select">` markup stable to preserve JS hook-up.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript in `librenms_sync.js` must not be wrapped in an IIFE and must use a master initializer `initializeScripts()` that runs on both `DOMContentLoaded` and `htmx:afterSwap` events.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript URL and tab state management must implement `initializeTabs()`, `getDeviceIdFromUrl()`, and `setInterfaceNameFieldFromURL()` to maintain browser state and URL synchronization.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript cache countdown functionality must implement `initializeCountdown()` and `initializeCountdowns()` functions to display and manage cache expiration timers.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/static/**/*.js : The `createCacheCountdown()` function is a generic countdown timer for cache expiration display.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📚 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/import_utils/vm_operations.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/jobs.py : `ImportDevicesJob` background job: imports devices and VMs. Must call `bulk_import_devices_shared()` for devices and `bulk_import_vms()` for VMs. `job.data` keys must include `imported_device_pks`, `imported_vm_pks`, `imported_libre_device_ids`, `imported_libre_vm_ids`, `server_key`, `total`, `success_count`, `failed_count`, `skipped_count`, `virtual_chassis_created`, `errors`, `completed`
Applied to files:
netbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_utils.py : `bulk_import_vms(vm_imports, user, ...)` must implement VM import
Applied to files:
netbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/tests/test_vm_operations.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 : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
Applied to files:
netbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tests/test_vm_operations.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 `get_virtual_chassis_member()` for port-to-member mapping and `get_librenms_sync_device()` for VC priority-based device selection in virtual chassis operations
Applied to files:
netbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tests/test_vm_operations.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/views/sync/**/*.py : Follow sync conventions defined in `.github/instructions/sync.instructions.md` for sync views, base views, tables, and sync JavaScript
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/sync/**/*.py : Sync action views must follow the pattern: check permissions with `LibreNMSPermissionMixin` and `NetBoxObjectPermissionMixin`, read selected items from `request.POST.getlist('select')`, load cached data using `CacheMixin.get_cache_key()`, apply changes inside `transaction.atomic()`, and redirect to the sync tab with `?tab=<resource>`.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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 : Hook into NetBox Django 5 plugin APIs via `navigation.py`, `urls.py`, and `api/` modules under `netbox_librenms_plugin/`; respect NetBox plugin conventions
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/views/**/*.py : Views must follow a three-layer structure: Base views in `views/base/`, Object sync views in `views/object_sync/`, and Sync action views in `views/sync/` with shared mixins from `views/mixins.py`
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/views/sync/**/*.py : Sync POST handlers must call `require_all_permissions()` (not just `require_write_permission()`) and return early if it returns a response; use `require_all_permissions_json()` for AJAX/JSON endpoints
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/{templates,tables}/**/*.{html,py} : Templates live in `templates/netbox_librenms_plugin/`; reuse/includes under `inc/`. Sync pages extend `librenms_sync_base.html`. Tables emit HTMX-enabled columns and buttons (`tables/*.py`), so prefer updating the table renderer in Python rather than templates when changing row actions.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/views/**/*.py : New views must extend the closest base class and compose mixins from `views/mixins.py` (`LibreNMSPermissionMixin`, `NetBoxObjectPermissionMixin`, `LibreNMSAPIMixin`, `CacheMixin`, `VlanAssignmentMixin`)
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/api/**/*.py : Update API serializers in `api/serializers.py` and `api/views.py` together to prevent contract drift between models and external API consumers
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : Base view classes (`BaseLibreNMSSyncView`, `BaseInterfaceTableView`, `BaseCableTableView`, `BaseIPAddressTableView`, `BaseVLANTableView`) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results with `CacheMixin` keys like `librenms_{data_type}_{model_name}_{pk}`, compare against NetBox objects, and render a django-tables2 table in a partial template.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py|**/views/object_sync/**/*.py|**/views/sync/**/*.py : Cache keys for sync data must follow the format: `librenms_{data_type}_{model_name}_{pk}` for fetched data and `librenms_{data_type}_last_fetched_{model_name}_{pk}` for fetch timestamps. VLAN group overrides must use `get_vlan_overrides_key(obj)`.
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch `cache` where imported: `netbox_librenms_plugin.views.imports.list.cache`, not `django.core.cache.cache`.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_vm_operations.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/api/views.py : API endpoints must use `LibreNMSPluginPermission` class in `api/views.py` where GET checks `view_librenmssettings` and other methods check `change_librenmssettings`
Applied to files:
netbox_librenms_plugin/views/sync/modules.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/views/**/*.py : All views must inherit `LibreNMSPermissionMixin` from `views/mixins.py` with `permission_required = PERM_VIEW_PLUGIN`
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/test_permissions.py : For JSON variants (`require_all_permissions_json`), assert `isinstance(response, JsonResponse)` and check `response.status_code == 403`.
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/static/**/*.js : Always check `response.ok` before processing fetch responses to catch HTTP errors. In catch blocks, show `error.message` for debugging rather than generic messages.
Applied to files:
netbox_librenms_plugin/views/sync/modules.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/views/**/*.py : Use `_get_safe_redirect_url(request)` to validate referrer URLs in permission checks to prevent open-redirect attacks
Applied to files:
netbox_librenms_plugin/views/sync/modules.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 : Always use exact-only matching (not fuzzy) for site, platform, device type, and role via functions in `utils.py` (`find_matching_site`, `match_librenms_hardware_to_device_type`, `find_matching_platform`)
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: All base view classes must follow the three-layer architecture: Base views define abstract data pipelines via `get_*_context()` methods, object sync views wire base views to NetBox models using `register_model_view()`, and sync action views handle POST requests with permissions checks and transactional updates.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : `VlanAssignmentMixin` must resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Test business logic via the utility modules (import_utils.py, import_validation_helpers.py, etc.) they call, not via HTTP requests, for views in `views/sync/`, `views/object_sync/`, and `views/imports/actions.py`.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/** : `BulkImportDevicesView` (POST) must execute import: background mode enqueues `ImportDevicesJob`; sync mode calls `bulk_import_devices()` + `bulk_import_vms()` and returns OOB row swaps with `HX-Trigger: closeModal`
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/**,**/import_utils.py : Recognize database Job status values as `pending`, `scheduled`, `running`, `completed`, `failed`, `errored` (NO `cancelled` status exists)
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_validation_helpers.py : Validation state mutation helpers in `import_validation_helpers.py` must include: `apply_role_to_validation()`, `apply_cluster_to_validation()`, `apply_rack_to_validation()` to update validation state when user selects role/cluster/rack
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/jobs.py,**/views/imports/** : Check `rq_job.is_stopped` or `rq_job.is_failed` flags in Redis for cancellation detection, not database status
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/**,**/import_utils.py : Recognize RQ status values as `queued`, `started`, `finished`, `stopped`, `failed` (NOT `completed`)
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
🪛 Biome (2.4.4)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
[error] 1417-1417: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🔇 Additional comments (7)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (2)
156-158: Module countdown interval wiring looks correct.Line 156 and Line 164 correctly mirror existing countdown lifecycle behavior (clear old interval, then reinitialize), preventing timer duplication on re-init.
Also applies to: 164-164
1432-1438: Idempotent submit-handler initialization is a solid fix.Line 1435-Line 1437 prevents stacked listeners across repeated
initializeScripts()runs, and Line 1456 ensures the handler is wired on both initial load and HTMX swaps.As per coding guidelines: JavaScript in
librenms_sync.jsmust use a master initializerinitializeScripts()that runs on bothDOMContentLoadedandhtmx:afterSwapevents.Also applies to: 1456-1456
netbox_librenms_plugin/tests/mock_librenms_server.py (3)
70-83: Shutdown behavior improvement is good.The
join(timeout=5)+is_alive()warning makes thread-leak failures visible during tests.
117-135: Ports default payload now matches expected API columns.Including
ifMtu,ifVlan, andifTrunkimproves fidelity withget_ports()expectations.
86-90: This comment is incorrect — the mock response shape is appropriate for the POST endpoint.The
add_device_response()method registers a POST/api/v0/devicesendpoint, not a list endpoint. The API client'sadd_device()method (lines 428–450) only checksresult.get("status") == "ok"and never accessesdevices,id, orhostnamefrom the response. The mock correctly reflects what the client code actually consumes.The confusion may stem from conflating GET
/api/v0/devices(list endpoint, which does return adevicesarray) with POST/api/v0/devices(create endpoint, which only requires astatusfield).> Likely an incorrect or invalid review comment.netbox_librenms_plugin/import_utils/vm_operations.py (2)
58-76: Good fix: VM creation and LibreNMS ID assignment are now atomic.This closes the partial-persistence gap between object creation and ID mapping.
172-179:server_keypropagation is correctly wired through validation and VM creation.This keeps per-server
librenms_idbehavior consistent across the import flow.Also applies to: 223-225
Comment on lines
+30
to
+41
| libre_device = { | ||
| "device_id": 1, | ||
| "hostname": "vm01.example.com", | ||
| "_computed_name": "vm01-computed", | ||
| } | ||
| mock_cluster = MagicMock() | ||
| mock_platform = MagicMock() | ||
| validation = { | ||
| "can_import": True, | ||
| "cluster": {"cluster": mock_cluster}, | ||
| "platform": {"platform": mock_platform}, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use shared tests/conftest.py fixtures instead of repeated ad-hoc mocks/data.
The inline mock/device/validation setup pattern is repeated heavily; switching to shared fixtures will reduce drift and maintenance cost.
Based on learnings: Reuse fixtures from tests/conftest.py instead of creating ad-hoc mocks (e.g., mock_librenms_api, sample_librenms_device, sample_validation_state_vm, and NetBox object fixtures).
Also applies to: 196-203, 280-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tests/test_vm_operations.py` around lines 30 - 41,
Replace the ad-hoc inline test data and MagicMock objects (e.g., libre_device,
mock_cluster, mock_platform, validation) with the shared fixtures from
tests/conftest.py: use mock_librenms_api, sample_librenms_device (instead of
libre_device), sample_validation_state_vm (instead of the validation dict), and
the NetBox object fixtures provided there; update test functions in
test_vm_operations.py (including the other occurrences around the previously
noted blocks) to accept these fixtures as parameters and remove the manual
MagicMock constructions so the tests rely on the centralized fixture instances.
Owner
Author
There was a problem hiding this comment.
won't fix, this is on purpose to make making separate PR easier.
There was a problem hiding this comment.
@marcinpsk, understood — keeping the tests self-contained makes it straightforward to cherry-pick them into separate PRs. Acknowledged!
🧠 Learnings used
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Reuse fixtures from `tests/conftest.py` instead of creating ad-hoc mocks: Configuration (`mock_multi_server_config`, `mock_legacy_config`), API client (`mock_librenms_api`), NetBox objects (`mock_netbox_device`, `mock_netbox_vm`, `mock_netbox_site`, `mock_netbox_platform`, `mock_netbox_device_type`, `mock_netbox_device_role`, `mock_netbox_cluster`, `mock_netbox_rack`), HTTP responses (`mock_response_factory`, `mock_success_response`, `mock_device_response`, `mock_error_response`, `mock_auth_error_response`), and Import workflow (`sample_librenms_device`, `sample_librenms_device_minimal`, `sample_validation_state`, `sample_validation_state_vm`).
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/test_netbox_librenms_plugin.py : `test_netbox_librenms_plugin.py` is an empty placeholder — do not add tests there.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_utils.py : `bulk_import_vms(vm_imports, user, ...)` must implement VM import
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`.
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 : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Never use `RequestFactory`—mock request objects directly or test method logic in isolation.
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 : Reuse `librenms_api.py` client for all LibreNMS communication instead of making direct `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and caching
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 : Always use exact-only matching (not fuzzy) for site, platform, device type, and role via functions in `utils.py` (`find_matching_site`, `match_librenms_hardware_to_device_type`, `find_matching_platform`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch `cache` where imported: `netbox_librenms_plugin.views.imports.list.cache`, not `django.core.cache.cache`.
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 `get_virtual_chassis_member()` for port-to-member mapping and `get_librenms_sync_device()` for VC priority-based device selection in virtual chassis operations
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/api/**/*.py : Update API serializers in `api/serializers.py` and `api/views.py` together to prevent contract drift between models and external API consumers
…s, hierarchy traversal, .get() guards
- modules_view.py: rename parent_idx (row index) to parent_row_idx to eliminate
variable collision with parent_ent_idx (entity physical index). table_data was
being indexed with an ENTITY-MIB index instead of the table row index, causing
potential IndexError or wrong-row mutations.
- modules_view.py: guard inv_by_index dict comprehension with .get() so malformed
rows missing entPhysicalIndex don't abort _merge_transceiver_data.
- modules_view.py/_collect_descendants: guard child_idx with .get() + skip None.
- sync/modules.py/_collect_branch: use .get('entPhysicalIndex') in parent lookup.
- sync/modules.py/_collect_children: guard child_idx with .get() + continue.
- sync/modules.py/_find_parent_module_id: replace for _ in range(10) with
visited-set while loop to detect cycles and avoid depth-limited misses.
- sync/modules.py/_match_bay: walk up full containment hierarchy for parent_name.
- sync/modules.py/_match_bay: accept preloaded exact/regex mappings, replace N+1
ORM queries per candidate with in-memory dict lookup.
- sync/modules.py/_install_single: accept optional exact/regex_mappings params.
- InstallBranchView/InstallSelectedView: preload ModuleBayMapping once per request.
- test_vm_operations.py: assert api.server_key is forwarded to create_vm_from_librenms.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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_vm_operations.py`:
- Around line 166-187: The test test_import_comment_contains_device_id currently
only asserts fixed substrings and never checks the LibreNMS device_id; update
the test so create_vm_from_librenms is invoked with the libre_device dict and
then assert that the created VM's comments (from
mock_vm_class.objects.create.call_args[1]["comments"]) contains the device_id
value (e.g., convert libre_device["device_id"] to string and assert it's
present) in addition to the existing "LibreNMS" and "netbox-librenms-plugin"
checks; ensure the assertion references create_vm_from_librenms and
mock_vm_class to locate the code under test.
- Around line 628-655: The test's statuses iterator currently yields an extra
"running" which makes the second checkpoint still be "running"; update the
iterator used by the _refresh side_effect (the variable statuses) so it yields
"running" for the first checkpoint and "failed" for the second (e.g., statuses =
iter(["running", "failed"])), keep the existing _refresh fallback that sets
mock_job.job.status = "failed" on StopIteration, and re-run the test using the
same mock_job and bulk_import_vms to ensure the log.info at the first checkpoint
and the failure at the second checkpoint occur as described.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 374-403: The recursive traversal in _collect_descendants scans
inventory_data on every call causing O(n²) behavior; fix by precomputing a
mapping of parent->children once in _build_context (e.g., build
children_by_parent keyed by entPhysicalContainedIn with lists of child dicts)
and then change _get_sub_components/_collect_descendants to use that mapping
instead of rebuilding children each recursion (either pass children_by_parent as
an argument to _get_sub_components and _collect_descendants or store it as an
instance attribute), and replace the line that computes children = [i for i in
inventory_data ...] with children = children_by_parent.get(parent_idx, []) while
keeping existing visited, depth handling and model checks intact.
- Around line 325-350: After appending the synthetic transceiver dict to
inventory_data in the transceiver loop, immediately update the dedupe maps so
future iterations won't append duplicates: add an entry for ent_idx into
inv_by_index (key ent_idx -> the synthetic dict) and, if serial is present, add
the serial to inv_serials (or map serial -> synthetic) to mirror how real
inventory rows are recorded; do this right after
inventory_data.append(synthetic) in the same block that builds the synthetic
item.
- Around line 78-82: The inventory cache is only object-scoped and needs to be
namespaced by LibreNMS server to avoid cross-server collisions; update the key
generation to include the server identifier and use that anywhere inventory keys
are built. Specifically, change calls that use self.get_cache_key(obj,
"inventory") (and corresponding cache.get/cache.delete sites around inventory
handling) to include the LibreNMS server id (e.g., self.librenms_api.server_id
or another unique server attribute) and update get_cache_key to accept and
incorporate that server identifier into the returned key so cache.set/get/delete
for inventory are scoped per server.
- Around line 583-589: The call to match.expand(mapping.netbox_bay_name) in the
module-bay resolution loop can raise re.error at runtime even though
ModuleBayMapping.clean() validates patterns; add defensive handling by wrapping
the match.expand(...) call in a try/except re.error block (inside the same scope
where match is checked) and skip that mapping on exception (optionally logging
the failure). Locate the expansion in views/base/modules_view.py where match =
re.fullmatch(mapping.librenms_name, name) and change the logic around
match.expand to catch re.error for mapping.netbox_bay_name so a bad runtime
expansion does not crash the view.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 402-405: _match_bay currently returns the first regex-derived bay
from BaseModuleTableView._lookup_regex_bay_mapping without validating FPC
branch, which can assign modules to the wrong FPC; after obtaining bay from
_lookup_regex_bay_mapping inside _match_bay, call the same FPC guard used in the
base matcher (BaseModuleTableView._fpc_slot_matches) with the same parameters
(re, name, phys_class, bay) and only return the bay if that check returns true,
otherwise continue searching for other candidates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b09c5e5-3f25-42c6-b6e3-9e67d0a004c4
📒 Files selected for processing (3)
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/modules.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
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/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
netbox_librenms_plugin/views/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/views/**/*.py: Views must follow a three-layer structure: Base views inviews/base/, Object sync views inviews/object_sync/, and Sync action views inviews/sync/with shared mixins fromviews/mixins.py
New views must extend the closest base class and compose mixins fromviews/mixins.py(LibreNMSPermissionMixin,NetBoxObjectPermissionMixin,LibreNMSAPIMixin,CacheMixin,VlanAssignmentMixin)
All views must inheritLibreNMSPermissionMixinfromviews/mixins.pywithpermission_required = PERM_VIEW_PLUGIN
Declarerequired_object_permissionsas a dict mapping HTTP methods to[(action, Model)]tuples for NetBox model operations; some views may set this dynamically per-request
Use_get_safe_redirect_url(request)to validate referrer URLs in permission checks to prevent open-redirect attacks
Files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
netbox_librenms_plugin/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/views/sync/**/*.py: Sync POST handlers must callrequire_all_permissions()(not justrequire_write_permission()) and return early if it returns a response; userequire_all_permissions_json()for AJAX/JSON endpoints
Follow sync conventions defined in.github/instructions/sync.instructions.mdfor sync views, base views, tables, and sync JavaScript
Files:
netbox_librenms_plugin/views/sync/modules.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/modules.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/modules_view.py
🧠 Learnings (41)
📓 Common learnings
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 : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
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/api/**/*.py : Update API serializers in `api/serializers.py` and `api/views.py` together to prevent contract drift between models and external API consumers
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
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: Sync pipelines must follow the pattern: fetch LibreNMS data via `librenms_api.py`, cache with `CacheMixin`, build comparison tables via `tables/*.py`, and render HTMX fragments from `templates/netbox_librenms_plugin/htmx/`
📚 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 : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/test_netbox_librenms_plugin.py : `test_netbox_librenms_plugin.py` is an empty placeholder — do not add tests there.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Reuse fixtures from `tests/conftest.py` instead of creating ad-hoc mocks: Configuration (`mock_multi_server_config`, `mock_legacy_config`), API client (`mock_librenms_api`), NetBox objects (`mock_netbox_device`, `mock_netbox_vm`, `mock_netbox_site`, `mock_netbox_platform`, `mock_netbox_device_type`, `mock_netbox_device_role`, `mock_netbox_cluster`, `mock_netbox_rack`), HTTP responses (`mock_response_factory`, `mock_success_response`, `mock_device_response`, `mock_error_response`, `mock_auth_error_response`), and Import workflow (`sample_librenms_device`, `sample_librenms_device_minimal`, `sample_validation_state`, `sample_validation_state_vm`).
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_utils.py : `bulk_import_vms(vm_imports, user, ...)` must implement VM import
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.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 `get_virtual_chassis_member()` for port-to-member mapping and `get_librenms_sync_device()` for VC priority-based device selection in virtual chassis operations
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch `cache` where imported: `netbox_librenms_plugin.views.imports.list.cache`, not `django.core.cache.cache`.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.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 : Hook into NetBox Django 5 plugin APIs via `navigation.py`, `urls.py`, and `api/` modules under `netbox_librenms_plugin/`; respect NetBox plugin conventions
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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 : Always use exact-only matching (not fuzzy) for site, platform, device type, and role via functions in `utils.py` (`find_matching_site`, `match_librenms_hardware_to_device_type`, `find_matching_platform`)
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.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/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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 : Reuse `librenms_api.py` client for all LibreNMS communication instead of making direct `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and caching
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.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 tests/**/*.py : Follow testing conventions and patterns defined in `.github/instructions/testing.instructions.md` for all test files
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.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/views/sync/**/*.py : Follow sync conventions defined in `.github/instructions/sync.instructions.md` for sync views, base views, tables, and sync JavaScript
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/api/**/*.py : Update API serializers in `api/serializers.py` and `api/views.py` together to prevent contract drift between models and external API consumers
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/jobs.py : `ImportDevicesJob` background job: imports devices and VMs. Must call `bulk_import_devices_shared()` for devices and `bulk_import_vms()` for VMs. `job.data` keys must include `imported_device_pks`, `imported_vm_pks`, `imported_libre_device_ids`, `imported_libre_vm_ids`, `server_key`, `total`, `success_count`, `failed_count`, `skipped_count`, `virtual_chassis_created`, `errors`, `completed`
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Test business logic via the utility modules (import_utils.py, import_validation_helpers.py, etc.) they call, not via HTTP requests, for views in `views/sync/`, `views/object_sync/`, and `views/imports/actions.py`.
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/**,**/import_utils.py : Recognize database Job status values as `pending`, `scheduled`, `running`, `completed`, `failed`, `errored` (NO `cancelled` status exists)
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/**,**/import_utils.py : Recognize RQ status values as `queued`, `started`, `finished`, `stopped`, `failed` (NOT `completed`)
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:00.657Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/jobs.py,**/views/imports/** : Check `rq_job.is_stopped` or `rq_job.is_failed` flags in Redis for cancellation detection, not database status
Applied to files:
netbox_librenms_plugin/tests/test_vm_operations.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/sync/**/*.py : Sync action views must follow the pattern: check permissions with `LibreNMSPermissionMixin` and `NetBoxObjectPermissionMixin`, read selected items from `request.POST.getlist('select')`, load cached data using `CacheMixin.get_cache_key()`, apply changes inside `transaction.atomic()`, and redirect to the sync tab with `?tab=<resource>`.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/views/**/*.py : Views must follow a three-layer structure: Base views in `views/base/`, Object sync views in `views/object_sync/`, and Sync action views in `views/sync/` with shared mixins from `views/mixins.py`
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/views/sync/**/*.py : Sync POST handlers must call `require_all_permissions()` (not just `require_write_permission()`) and return early if it returns a response; use `require_all_permissions_json()` for AJAX/JSON endpoints
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/{templates,tables}/**/*.{html,py} : Templates live in `templates/netbox_librenms_plugin/`; reuse/includes under `inc/`. Sync pages extend `librenms_sync_base.html`. Tables emit HTMX-enabled columns and buttons (`tables/*.py`), so prefer updating the table renderer in Python rather than templates when changing row actions.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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: Keep REST endpoint server responses and HTMX targets in sync; implement REST endpoints in `views/imports/actions.py` and HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/views/sync/modules.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/views/**/*.py : New views must extend the closest base class and compose mixins from `views/mixins.py` (`LibreNMSPermissionMixin`, `NetBoxObjectPermissionMixin`, `LibreNMSAPIMixin`, `CacheMixin`, `VlanAssignmentMixin`)
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: All base view classes must follow the three-layer architecture: Base views define abstract data pipelines via `get_*_context()` methods, object sync views wire base views to NetBox models using `register_model_view()`, and sync action views handle POST requests with permissions checks and transactional updates.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py|**/views/object_sync/**/*.py|**/views/sync/**/*.py : Cache keys for sync data must follow the format: `librenms_{data_type}_{model_name}_{pk}` for fetched data and `librenms_{data_type}_last_fetched_{model_name}_{pk}` for fetch timestamps. VLAN group overrides must use `get_vlan_overrides_key(obj)`.
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : Base view classes (`BaseLibreNMSSyncView`, `BaseInterfaceTableView`, `BaseCableTableView`, `BaseIPAddressTableView`, `BaseVLANTableView`) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results with `CacheMixin` keys like `librenms_{data_type}_{model_name}_{pk}`, compare against NetBox objects, and render a django-tables2 table in a partial template.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py|**/views/object_sync/**/*.py : Use `LibreNMSAPIMixin` to provide lazy-loaded `LibreNMSAPI` instance via `self.librenms_api` property and `get_server_info()` method for template context.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.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/api/views.py : API endpoints must use `LibreNMSPluginPermission` class in `api/views.py` where GET checks `view_librenmssettings` and other methods check `change_librenmssettings`
Applied to files:
netbox_librenms_plugin/views/sync/modules.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/views/**/*.py : All views must inherit `LibreNMSPermissionMixin` from `views/mixins.py` with `permission_required = PERM_VIEW_PLUGIN`
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/librenms_sync.js : JavaScript verification functions must include `handleInterfaceChange()`, `handleCableChange()`, `handleVRFChange()` that POST to single-item verify endpoints to validate resource changes.
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:51.364Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/test_permissions.py : For JSON variants (`require_all_permissions_json`), assert `isinstance(response, JsonResponse)` and check `response.status_code == 403`.
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/static/**/*.js : Always check `response.ok` before processing fetch responses to catch HTTP errors. In catch blocks, show `error.message` for debugging rather than generic messages.
Applied to files:
netbox_librenms_plugin/views/sync/modules.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/views/**/*.py : Use `_get_safe_redirect_url(request)` to validate referrer URLs in permission checks to prevent open-redirect attacks
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:18.134Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-03-03T13:24:18.134Z
Learning: Applies to netbox_librenms_plugin/templates/**/htmx/*import*.html : Device import dropdowns rely on TomSelect decorators set up elsewhere. Keep `<select class="device-role-select">` markup stable to preserve JS hook-up.
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-03T13:24:38.082Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/sync.instructions.md:0-0
Timestamp: 2026-03-03T13:24:38.082Z
Learning: Applies to **/views/base/**/*.py : `VlanAssignmentMixin` must resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Applied to files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/base/modules_view.py
🔇 Additional comments (4)
netbox_librenms_plugin/tests/test_vm_operations.py (2)
293-326: Good coverage improvement forserver_keyforwarding.The explicit assertion on Line 325 closes an important regression vector in VM creation call wiring.
313-314: Patch target selection for inline-imported helpers is correct.These tests patch
apply_cluster_to_validation/apply_role_to_validationat their source module, which is the right pattern for deferred imports.Based on learnings: Patch deferred/inline imports at their source module (e.g.,
netbox_librenms_plugin.import_utils.process_device_filters), not the consuming module.Also applies to: 366-367, 413-414
netbox_librenms_plugin/views/sync/modules.py (2)
24-25: Permission guard usage is correct for sync POST handlers.Line 24, Line 76, and Line 429 correctly use
require_all_permissions("POST")with early return for these non-JSON flows.As per coding guidelines: “Sync POST handlers must call
require_all_permissions()(not justrequire_write_permission()) and return early if it returns a response; userequire_all_permissions_json()for AJAX/JSON endpoints.”Also applies to: 76-77, 429-430
113-119: Good improvement: preloading bay mappings avoids per-item ORM churn.Lines 113-119 and Lines 465-470 correctly batch-load
ModuleBayMappingand pass exact/regex collections through the install pipeline.Also applies to: 465-470
…comments device_id, tests - CacheMixin.get_cache_key() accepts optional server_key param for per-server namespacing - modules_view: inventory cache set/get/ttl use server_key from librenms_api - tables/modules: LibreNMSModuleTable stores server_key, passes it in inline install forms - sync/modules: install views read server_key from POST, use it for cache.get/delete - _module_sync_content.html: add server_key hidden input to install-selected-form - object_sync/devices: DeviceModuleTableView.get_table passes server_key to table - modules_view: _collect_descendants refactored to O(n) via precomputed children_by_parent - modules_view: synthetic transceiver deduplication: update inv_by_index/inv_serials after append - modules_view: match.expand() wrapped in try/except re.error to prevent runtime crash - vm_operations: comments string includes device_id for traceability - test_modules_view: _make_view sets _librenms_api mock; _collect_descendants tests use new signature - test_sync_modules: regression test for parent_row_idx vs entity index collision - test_vm_operations: statuses iterator fix; device_id assertion in comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
netbox_librenms_plugin/views/sync/modules.py (1)
197-212: 🧹 Nitpick | 🔵 TrivialDe-duplicate
_get_module_types()to prevent cross-layer drift.This lookup logic now exists in both sync and base modules views. A shared helper would reduce future mismatch risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/modules.py` around lines 197 - 212, The _get_module_types function duplicate should be moved into a single shared helper and both callers updated to use it: extract the logic in _get_module_types (iterating ModuleType, adding mt.model and mt.part_number, applying ModuleTypeMapping entries) into a new helper function (e.g., get_module_types_indexed_by_model) in a common module shared by sync and base modules views, replace the local _get_module_types implementation with an import and call to that helper in netbox_librenms_plugin.views.sync.modules, and update the other view that currently duplicates this logic to import and call the same helper so the mapping logic is centralized and not duplicated.netbox_librenms_plugin/tables/modules.py (1)
39-41:⚠️ Potential issue | 🟠 MajorAdd required row-level
data-*attributes toMeta.row_attrs.
row_attrscurrently exposes only CSS class; the sync-table contract expects machine-readable row metadata too.♻️ Proposed fix
class Meta: attrs = {"class": "table table-hover object-list", "id": "librenms-module-table"} - row_attrs = {"class": lambda record: record.get("row_class", "")} + row_attrs = { + "class": lambda record: record.get("row_class", ""), + "data-ent-physical-index": lambda record: record.get("ent_physical_index", ""), + "data-status": lambda record: record.get("status", ""), + }As per coding guidelines: “Table classes in
tables/must … includedata-*attributes in row attrs.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/modules.py` around lines 39 - 41, Meta.row_attrs only sets the CSS class but must also expose machine-readable row metadata for the sync-table contract; update the Meta class in netbox_librenms_plugin/tables/modules.py (the Meta and row_attrs symbols) so row_attrs returns a dict that merges the existing "class" with required data-* attributes (e.g., data-id, data-pk, data-name or other fields your sync uses) by pulling values from each record (using record.get(...)) — keep the lambda signature but return a mapping like {"class": ..., "data-id": ..., "data-pk": ..., "data-name": ...} so downstream code can read those data-* attributes.netbox_librenms_plugin/views/base/modules_view.py (2)
381-395:⚠️ Potential issue | 🟠 MajorAvoid rebuilding
children_by_parentfor every top-level item traversal.
_get_sub_components()reconstructs the full parent→children map each call, which reintroduces unnecessary repeated full scans on larger inventories.♻️ Proposed fix
- def _get_sub_components(self, parent_idx, inventory_data): + def _get_sub_components(self, parent_idx, children_by_parent): @@ - children_by_parent: dict = {} - for item in inventory_data: - p = item.get("entPhysicalContainedIn") - if p is not None: - children_by_parent.setdefault(p, []).append(item) - results = [] self._collect_descendants(parent_idx, children_by_parent, depth=1, results=results, visited={parent_idx}) return results+ children_by_parent: dict = {} + for inv_item in inventory_data: + p = inv_item.get("entPhysicalContainedIn") + if p is not None: + children_by_parent.setdefault(p, []).append(inv_item) @@ - sub_items = self._get_sub_components(parent_ent_idx, inventory_data) + sub_items = self._get_sub_components(parent_ent_idx, children_by_parent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 381 - 395, Avoid rebuilding the children_by_parent map inside _get_sub_components on every call: compute children_by_parent once and reuse it when traversing multiple top-level items. Refactor so _get_sub_components accepts a precomputed children_by_parent (or cache it on the instance as self._children_by_parent mapped from the inventory_data identity) and then call self._collect_descendants(parent_idx, children_by_parent, ...). Update callers to supply the precomputed map or ensure the cache is refreshed when inventory_data changes.
481-489:⚠️ Potential issue | 🟠 MajorAlign parent-container name resolution with sync install matching.
Base table matching only checks immediate parent name, while install matching walks ancestors. This can desync “matched/unmatched” UI status from actual install behavior for nested items under unnamed containers.
♻️ Proposed fix
def _find_parent_container_name(self, item, index_map): """Resolve the parent container name for an inventory item.""" - contained_in = item.get("entPhysicalContainedIn", 0) - if contained_in == 0: - return None - parent = index_map.get(contained_in) - if parent: - return parent.get("entPhysicalName", "") + current_idx = item.get("entPhysicalContainedIn", 0) + visited = set() + while current_idx and current_idx not in visited: + visited.add(current_idx) + parent = index_map.get(current_idx) + if not parent: + break + name = parent.get("entPhysicalName", "") + if name: + return name + current_idx = parent.get("entPhysicalContainedIn", 0) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 481 - 489, _find_parent_container_name currently returns only the immediate parent's entPhysicalName, causing mismatch with install logic that walks ancestor chains; update _find_parent_container_name to iterate up the entPhysicalContainedIn chain using index_map: start from item["entPhysicalContainedIn"], loop while contained_in != 0, lookup parent = index_map.get(contained_in), if parent and parent.get("entPhysicalName") is non-empty return that name, otherwise set contained_in = parent.get("entPhysicalContainedIn", 0) and continue until no parent found, then return None; ensure you reference the entPhysicalContainedIn and entPhysicalName keys and the index_map and item parameters in the implementation.netbox_librenms_plugin/tests/test_tables_modules.py (1)
354-441: 🧹 Nitpick | 🔵 TrivialConsolidate repeated ad-hoc
devicemocks into a shared fixture/helper.Lines 356-357, 365-366, 384-385, 400-401, 419-420, and 433-434 repeat the same manual
MagicMock()setup. Please switch these action tests to a shared fixture/helper and pass it into_make_table(...)to reduce drift.Based on learnings: Reuse fixtures from
tests/conftest.pyinstead of creating ad-hoc mocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_tables_modules.py` around lines 354 - 441, Replace the repeated ad-hoc device = MagicMock(); device.pk = X setups in the tests that call _make_table(...) with a shared fixture or helper from tests/conftest.py (e.g., reuse the provided device fixture) and pass that fixture into _make_table; update tests that need different pk values to either set device.pk on the shared fixture before calling _make_table or implement a small helper fixture that returns a MagicMock with a configurable pk, then use that helper in test_render_actions_no_buttons_returns_empty_string, test_render_actions_can_install_renders_install_button, test_render_actions_has_installable_children_renders_branch_button, test_render_actions_both_buttons_rendered, test_render_actions_installable_children_without_index_skips_branch, and test_render_actions_csrf_token_included_in_form so each calls _make_table(device=device_fixture) instead of creating its own MagicMock.netbox_librenms_plugin/tests/test_modules_view.py (1)
58-61:⚠️ Potential issue | 🟡 MinorAvoid mutating mock queryset
__class__; use a configured mock object instead.Line 61’s
__class__ = listhack is brittle and can silently break when queryset interactions change. Configurefirst,__iter__, and__len__on a dedicated mock and return that fromfilter().♻️ Safer mock setup
- mock_mapping.objects.filter.return_value.first.return_value = None - mock_mapping.objects.filter.return_value.__iter__ = lambda s: iter([]) - mock_mapping.objects.filter.return_value.__list__ = lambda s: [] - # list() is called on the queryset for regex mappings - mock_mapping.objects.filter.return_value.__class__ = list + filter_result = MagicMock() + filter_result.first.return_value = None + filter_result.__iter__ = MagicMock(return_value=iter([])) + filter_result.__len__ = MagicMock(return_value=0) + mock_mapping.objects.filter.return_value = filter_result#!/bin/bash # Verify usage patterns in the implementation and locate fragile mock patterns in tests. rg -n -C3 'ModuleBayMapping|list\(' netbox_librenms_plugin/views/base/modules_view.py rg -n -C2 '__class__\s*=\s*list|__list__\s*=' netbox_librenms_plugin/tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_modules_view.py` around lines 58 - 61, Replace the brittle assignment of __class__ on the mocked queryset by constructing a dedicated mock queryset object, configure its iterator, length and first-return behavior, and return that from mock_mapping.objects.filter(); specifically, create a mock (e.g., mock_qs), set mock_qs.__iter__ to yield an empty iterator, set mock_qs.__len__ to return 0 (so list(mock_qs) works), set mock_qs.first to return None (or appropriate sentinel), then assign mock_mapping.objects.filter.return_value = mock_qs so tests referencing filter(), list(), iteration or first() use the configured mock instead of mutating __class__.
🤖 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_vm_operations.py`:
- Around line 510-547: Update the
test_sync_options_use_sysname_and_strip_domain_forwarded test to also assert
that the API server_key is forwarded to validate_device_for_import: after
calling bulk_import_vms and retrieving mock_validate.call_args[1], add an
assertion that call_kwargs["server_key"] equals mock_api.server_key
(mock_api.server_key was set to "default" earlier) so the mocked
validate_device_for_import receives the server_key parameter.
---
Duplicate comments:
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 39-41: Meta.row_attrs only sets the CSS class but must also expose
machine-readable row metadata for the sync-table contract; update the Meta class
in netbox_librenms_plugin/tables/modules.py (the Meta and row_attrs symbols) so
row_attrs returns a dict that merges the existing "class" with required data-*
attributes (e.g., data-id, data-pk, data-name or other fields your sync uses) by
pulling values from each record (using record.get(...)) — keep the lambda
signature but return a mapping like {"class": ..., "data-id": ..., "data-pk":
..., "data-name": ...} so downstream code can read those data-* attributes.
In `@netbox_librenms_plugin/tests/test_modules_view.py`:
- Around line 58-61: Replace the brittle assignment of __class__ on the mocked
queryset by constructing a dedicated mock queryset object, configure its
iterator, length and first-return behavior, and return that from
mock_mapping.objects.filter(); specifically, create a mock (e.g., mock_qs), set
mock_qs.__iter__ to yield an empty iterator, set mock_qs.__len__ to return 0 (so
list(mock_qs) works), set mock_qs.first to return None (or appropriate
sentinel), then assign mock_mapping.objects.filter.return_value = mock_qs so
tests referencing filter(), list(), iteration or first() use the configured mock
instead of mutating __class__.
In `@netbox_librenms_plugin/tests/test_tables_modules.py`:
- Around line 354-441: Replace the repeated ad-hoc device = MagicMock();
device.pk = X setups in the tests that call _make_table(...) with a shared
fixture or helper from tests/conftest.py (e.g., reuse the provided device
fixture) and pass that fixture into _make_table; update tests that need
different pk values to either set device.pk on the shared fixture before calling
_make_table or implement a small helper fixture that returns a MagicMock with a
configurable pk, then use that helper in
test_render_actions_no_buttons_returns_empty_string,
test_render_actions_can_install_renders_install_button,
test_render_actions_has_installable_children_renders_branch_button,
test_render_actions_both_buttons_rendered,
test_render_actions_installable_children_without_index_skips_branch, and
test_render_actions_csrf_token_included_in_form so each calls
_make_table(device=device_fixture) instead of creating its own MagicMock.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 381-395: Avoid rebuilding the children_by_parent map inside
_get_sub_components on every call: compute children_by_parent once and reuse it
when traversing multiple top-level items. Refactor so _get_sub_components
accepts a precomputed children_by_parent (or cache it on the instance as
self._children_by_parent mapped from the inventory_data identity) and then call
self._collect_descendants(parent_idx, children_by_parent, ...). Update callers
to supply the precomputed map or ensure the cache is refreshed when
inventory_data changes.
- Around line 481-489: _find_parent_container_name currently returns only the
immediate parent's entPhysicalName, causing mismatch with install logic that
walks ancestor chains; update _find_parent_container_name to iterate up the
entPhysicalContainedIn chain using index_map: start from
item["entPhysicalContainedIn"], loop while contained_in != 0, lookup parent =
index_map.get(contained_in), if parent and parent.get("entPhysicalName") is
non-empty return that name, otherwise set contained_in =
parent.get("entPhysicalContainedIn", 0) and continue until no parent found, then
return None; ensure you reference the entPhysicalContainedIn and entPhysicalName
keys and the index_map and item parameters in the implementation.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 197-212: The _get_module_types function duplicate should be moved
into a single shared helper and both callers updated to use it: extract the
logic in _get_module_types (iterating ModuleType, adding mt.model and
mt.part_number, applying ModuleTypeMapping entries) into a new helper function
(e.g., get_module_types_indexed_by_model) in a common module shared by sync and
base modules views, replace the local _get_module_types implementation with an
import and call to that helper in netbox_librenms_plugin.views.sync.modules, and
update the other view that currently duplicates this logic to import and call
the same helper so the mapping logic is centralized and not duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b40bd7c-6b05-4015-b91b-73d1d54736dd
📒 Files selected for processing (11)
netbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
…s, optimize _get_sub_components
- InstallModuleView/InstallBranchView/InstallSelectedView: remove cache.delete()
calls after successful install. The LibreNMS inventory cache is unaffected by
NetBox module installs; _get_module_bays() is a live DB query so the next render
already reflects the installed state. Deleting the cache caused an empty modules
tab after install (regression).
- Extract _get_module_types() logic to utils.get_module_types_indexed() and make
both BaseModuleTableView._get_module_types() and InstallBranchView._get_module_types()
thin wrappers. Eliminates duplicated implementation.
- Precompute children_by_parent dict once in _build_context() and pass it to
_get_sub_components(). Removes O(n) rebuild per top-level item; sub-component
traversal is now O(n) total across all top-level items.
- Fix _find_parent_container_name() to walk the full ancestor containment chain
instead of only checking the immediate parent. Items with intermediate ancestors
that have empty entPhysicalName now correctly resolve to the nearest named ancestor.
- Fix Biome lint: expression-body arrow el => el.remove() -> block-body
el => { el.remove(); } in librenms_sync.js.
- Fix brittle __class__ = list mock in test_modules_view.py: replace with a
properly configured MagicMock queryset.
- test_vm_operations.py: assert server_key is forwarded to validate_device_for_import.
- test_sync_modules.py: add TestInstallViewsDoNotDeleteCache regression tests.
marcinpsk
added a commit
that referenced
this pull request
Mar 28, 2026
- #12: Pass manufacturer as explicit parameter to _build_row/_build_table_rows; remove self._device_manufacturer instance attribute (hidden temporal coupling) - #13: Eliminate second resolve_module_type loop in _build_table_rows; track installable flag inline during first pass via sub_row.get('module_type_id') - #14: Pre-compute ignore_cache dict once in _build_context; pass to _find_transparent_indices and _collect_top_items instead of double evaluation - #15: Fix convert_speed_to_kbps return type annotation to -> int | None - #16: Extract _apply_rules() inner helper in apply_normalization_rules to eliminate duplicated regex loop between manufacturer/non-manufacturer branches - #17/#18: Extract BaseSNMPForm with 6 shared fields; rename AddToLIbreSNMPV1V2 -> AddToLibreSNMPV1V2 and AddToLIbreSNMPV3 -> AddToLibreSNMPV3 (typo fix); add backwards-compatible aliases for existing imports - #19: Rename related_name='librenms_mappings' -> 'librenms_device_type_mappings' on DeviceTypeMapping.netbox_device_type and 'librenms_module_type_mappings' on ModuleTypeMapping.netbox_module_type to avoid ambiguity; migration 0011 - #20: Add filterset_class to all six API ViewSets (InterfaceTypeMapping, DeviceTypeMapping, ModuleTypeMapping, ModuleBayMapping, NormalizationRule, InventoryIgnoreRule)
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
Documentation
Bug Fixes
Tests / CI