Consolidate the mapping and rule menus into two tabbed sidebar entries - #122
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (17)📚 Learning: 2026-03-07T22:46:57.537ZApplied to files:
📚 Learning: 2026-03-08T13:09:49.031ZApplied to files:
📚 Learning: 2026-05-05T09:51:15.707ZApplied to files:
📚 Learning: 2026-06-01T13:35:47.228ZApplied to files:
📚 Learning: 2026-07-01T16:41:50.451ZApplied to files:
📚 Learning: 2026-07-30T02:40:53.531ZApplied to files:
📚 Learning: 2026-08-03T19:16:41.198ZApplied to files:
📚 Learning: 2026-03-27T02:04:22.276ZApplied to files:
📚 Learning: 2026-06-02T11:11:56.131ZApplied to files:
📚 Learning: 2026-06-02T20:43:51.604ZApplied to files:
📚 Learning: 2026-06-15T18:49:04.201ZApplied to files:
📚 Learning: 2026-06-17T07:31:54.849ZApplied to files:
📚 Learning: 2026-06-19T14:03:09.440ZApplied to files:
📚 Learning: 2026-06-25T07:07:59.192ZApplied to files:
📚 Learning: 2026-07-02T21:46:46.384ZApplied to files:
📚 Learning: 2026-04-01T15:55:42.180ZApplied to files:
📚 Learning: 2026-05-05T09:46:17.700ZApplied to files:
🪛 ast-grep (0.45.0)netbox_librenms_plugin/tests/test_validation_template_server_key.py[warning] 27-27: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling. (redos-non-literal-regex-python) 🔇 Additional comments (3)
📝 WalkthroughWalkthroughThis PR adds bulk collision pre-checking, permission-scoped object access, cache and linkage validation, shared value normalization, and consolidated mapping and rule navigation. ChangesImport safety and execution
Consolidated navigation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
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/bulk_import.py (1)
897-936: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatched VM's cluster is never populated (asymmetry with the device_role branch).
For a device match,
apply_role_to_validation(validation, new_device.role, ...)populatesdevice_rolewith the matched device's actual role for display. For a VM match (actual_is_vm=True), there's no equivalent call to setvalidation["cluster"]to the matched VM's actual cluster — the field is left at whatever stale/default value it had before.can_import/is_readyare still correctly forcedFalseafterward, so this doesn't affect import gating, but it leaves the "existing match" UI showing an unselected/empty cluster for a VM that in fact already has one, unlike the device-role display.🐛 Proposed fix for VM cluster symmetry
if not actual_is_vm and hasattr(new_device, "role") and new_device.role: apply_role_to_validation(validation, new_device.role, is_vm=False) elif not actual_is_vm: validation["device_role"] = { "found": False, "role": None, "available_roles": validation.get("device_role", {}).get("available_roles", []), } + elif actual_is_vm and hasattr(new_device, "cluster") and new_device.cluster: + apply_cluster_to_validation(validation, new_device.cluster) + elif actual_is_vm: + validation["cluster"] = { + "found": False, + "cluster": None, + "available_clusters": validation.get("cluster", {}).get("available_clusters", []), + } recalculate_validation_status(validation, is_vm=actual_is_vm)
apply_cluster_to_validationis already defined inimport_validation_helpers.py; ensure it's imported here alongsideapply_role_to_validation.Based on learnings: keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views — the same symmetry principle applies to reflecting the matched object's actual selection, not just the option list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/import_utils/bulk_import.py` around lines 897 - 936, The existing-match handling in bulk_import.py is asymmetric: the device path populates the matched role for display, but the VM path never sets the matched cluster, leaving stale or empty UI state. Update the existing-device branch around _refresh_librenms_linkage and recalculate_validation_status to call apply_cluster_to_validation for actual VM matches, and ensure it is imported alongside apply_role_to_validation so validation["cluster"] reflects the resolved VM’s real cluster while keeping available_clusters/available_roles populated.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/usage_tips/interface_mappings.md`:
- Line 32: The screenshot markdown in the interface mappings doc is missing alt
text, which triggers the MD045 warning. Update the image syntax to include a
meaningful description of the screenshot content instead of an empty alt field,
keeping the existing image reference and width option intact.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 492-521: The handling of merge_candidates is inconsistent:
_clear_existing_match_derived_fields removes the key entirely, while
apply_oob_detection_result and validate_device_for_import keep it present with a
None value. Update _clear_existing_match_derived_fields to preserve the existing
contract by setting validation["merge_candidates"] to None instead of popping
it, so downstream code using direct key access does not hit a KeyError.
In `@netbox_librenms_plugin/tables/interfaces.py`:
- Around line 303-313: The OOB badge check in render_name is duplicated in
another table renderer, so centralize it in a shared helper. Add a small utility
like oob_badge_html(record) near render_vc_member_options in utils.py, move the
record.get("_source") == "oob" logic and OOB_BADGE_HTML markup there, then
update render_name (and the matching code in tables/modules.py) to call the
helper instead of building the badge inline.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Around line 14-25: The migrated interface-sync branch in
_interface_sync_content.html currently renders only the CSRF token, but
handleInterfaceChange() expects a server_key input as well. Update the
migrated-mode block so it also emits a hidden server_key field alongside
csrfmiddlewaretoken, ensuring the JS-driven verify-interface flow stays scoped
to the correct server on non-default deployments.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.html`:
- Around line 19-40: The `_module_sync_content.html` template still duplicates
the same `module_sync.server_key` hidden input logic in both the install form
branch and the migrated branch. Replace both inline conditionals with the shared
`inc/_hidden_server_key.html` partial, matching the reuse already done in
`_module_sync.html` and `_vlan_sync.html`, so `module_sync.server_key` is
rendered through one central template path.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html`:
- Line 35: The VLAN sync template is duplicating the hidden server key input
pattern instead of reusing the shared partial. Update _vlan_sync_content.html to
render inc/_hidden_server_key.html the same way _module_sync_content.html does,
using the vlan_sync server key context so the hidden-input contract stays
single-sourced and consistent.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 748-761: The “Add as OOB” form markup is duplicated in the
`device_validation_details.html` template across the `serial` and `primary_ip`
match branches, which risks drift. Extract the shared form block used in
`device_add_as_oob`, including the CSRF token, `existing_device_id`,
`_hidden_server_key.html`, `_oob_interface_select.html`, and submit button, into
a reusable partial and include it from both branches to keep behavior
consistent.
In `@netbox_librenms_plugin/tests/test_coverage_bulk_import.py`:
- Around line 806-818: The `_device_validation()` test fixture is missing the
always-present "cluster" entry that real `validate_device_for_import()` results
include, which can lead to a future KeyError in `recalculate_validation_status`
when cross-model VM/device matching accesses `validation["cluster"]["found"]`.
Update the baseline dict in `_device_validation()` to include a realistic
"cluster" key alongside the existing `device_role` fields so any tests using
this helper match the actual validation shape.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 897-936: The existing-match handling in bulk_import.py is
asymmetric: the device path populates the matched role for display, but the VM
path never sets the matched cluster, leaving stale or empty UI state. Update the
existing-device branch around _refresh_librenms_linkage and
recalculate_validation_status to call apply_cluster_to_validation for actual VM
matches, and ensure it is imported alongside apply_role_to_validation so
validation["cluster"] reflects the resolved VM’s real cluster while keeping
available_clusters/available_roles populated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3d8d7f6-9d6f-45b4-bd7f-410699606f63
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (127)
.gitignoredocs/SUMMARY.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/custom_field.mddocs/usage_tips/interface_mappings.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/oob_management.mdmkdocs.ymlnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_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/_vlan_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_existing_librenms_link_status.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_oob_interface_select.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_hidden_server_key.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_migrate_move_button.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/tests/_html_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_cable_sync_content_template.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_api.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_device_fields_server_scoping.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_api_server_config.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_module_sync_content_template.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_multiserver_get_cache_scoping.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_rules_patterns_navigation.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_page_server_key_forms.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_sync_view_unresolved_vc.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_utils_shared_helpers.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 7, 2026 11:29
537e76b to
98de27d
Compare
marcinpsk
commented
Jul 7, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 7, 2026 13:52
98de27d to
b25eb79
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 7, 2026 22:15
b25eb79 to
bb48f0e
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
466-1382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffVery large, deeply-nested function.
validate_device_for_import()now spans ~900 lines with many interleaved terminal-state branches (ambiguous id, duplicate hostname/serial, duplicate IP, merge-candidate detection). The logic itself is correct and thoroughly commented/tested, but the sheer size makes it hard to reason about future changes safely. Consider extracting self-contained stages (e.g., the "Stage 1 duplicate-peer" check and "Stage 2 merge-candidate" block at lines 873-1044) into named helper functions once the current stack of dependent PRs settles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 466 - 1382, validate_device_for_import() has grown into a very large, deeply nested validation flow, making future changes risky. Split the self-contained terminal-state sections into named helpers, especially the Stage 1 duplicate-peer guard and Stage 2 merge-candidate logic inside validate_device_for_import, so the main function reads as a high-level orchestration over smaller units with clear responsibilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 971-983: The collision check in detect_collisions_for_device_ids
should reuse and persist the LibreNMS rows it fetches instead of only reading
from libre_devices_cache. Update the helper so any cache misses are written back
into the shared cache object (or otherwise returned to the caller) before the
import flow continues, and ensure the actions.py import path keeps using that
populated cache for the later device-import step.
- Around line 3311-3324: The merge flow in the candidate/device selection logic
resolves `winner_sync` and `donor_sync` too early in relation to the transaction
lock. Move the `get_librenms_sync_device()` lookups into the atomic section
after the candidate devices are locked, or extend the lock to cover the
VC-related rows used by that lookup, so `winner_sync`/`donor_sync` are computed
from current state before the merge write.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 466-1382: validate_device_for_import() has grown into a very
large, deeply nested validation flow, making future changes risky. Split the
self-contained terminal-state sections into named helpers, especially the Stage
1 duplicate-peer guard and Stage 2 merge-candidate logic inside
validate_device_for_import, so the main function reads as a high-level
orchestration over smaller units with clear responsibilities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19a8e86a-377f-422a-9e1b-b8203ebc6e8e
📒 Files selected for processing (33)
docs/usage_tips/interface_mappings.mddocs/usage_tips/mapping_rules.mdnetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_rules_patterns_navigation.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 8, 2026 15:56
bb48f0e to
5c22a7a
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 9, 2026 04:47
5c22a7a to
f926c98
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/views/imports/actions.py (1)
1047-1060: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not queue Django messages on the HTMX success path.
messages.info(...)runs even whenis_htmxis true, but this view returns an HX redirect instead of rendering the message queue. That leaks the flash message into the next full-page request and contradicts the surrounding comment.♻️ Proposed fix
- messages.info( - request, - format_html( - "Import job started for {} device{}. " - 'You can monitor progress in the <a href="{}">Jobs interface</a>.', - total_import_count, - "s" if total_import_count != 1 else "", - job_url, - ), - ) + if not is_htmx: + messages.info( + request, + format_html( + "Import job started for {} device{}. " + 'You can monitor progress in the <a href="{}">Jobs interface</a>.', + total_import_count, + "s" if total_import_count != 1 else "", + job_url, + ), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/imports/actions.py` around lines 1047 - 1060, The success path in the import action is always calling messages.info even for HTMX requests, which can leak a flash message into the next full-page load. Update the logic around the job_url and messages.info block in actions.py so that Django messages are only queued on the non-HTMX path, while the is_htmx branch returns the HX redirect without adding a message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1047-1060: The success path in the import action is always calling
messages.info even for HTMX requests, which can leak a flash message into the
next full-page load. Update the logic around the job_url and messages.info block
in actions.py so that Django messages are only queued on the non-HTMX path,
while the is_htmx branch returns the HX redirect without adding a message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 840c3a67-69ab-4086-9be6-a19922abedc4
📒 Files selected for processing (33)
docs/usage_tips/interface_mappings.mddocs/usage_tips/mapping_rules.mdnetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_rules_patterns_navigation.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/modules.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments live in
templates/netbox_librenms_plugin/htmx/including:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/navigation.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/views/imports/actions.py
**/import_utils/__init__.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
import_utils/is a package; the__init__.pymust re-export key functions so callers can usefrom import_utils import ...for functions fromfilters.py,device_operations.py,vm_operations.py,cache.py,permissions.py, andvirtual_chassis.py
Files:
netbox_librenms_plugin/import_utils/__init__.py
**/jobs.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/jobs.py: Background jobs must use NetBox'sJobRunnerbase class (netbox.jobs.JobRunner) for long-running operations like device filtering with VC detection
Use Job UUID (job.job_id) for RQ API endpoints (/api/core/background-tasks/{uuid}/) and Job PK (job.pk) for database endpoints and result loading
RQ status values are:queued,started,finished,stopped,failed(NOTcompleted). Database Job status values are:pending,scheduled,running,completed,failed,errored(NOcancelledstatus exists)
Checkrq_job.is_stoppedorrq_job.is_failedflags in Redis for cancellation detection, not database status
FilterDevicesJobbackground job — filters devices with VC detection.job.datakeys:device_ids,total_processed,filters,server_key,vc_detection_enabled,cache_timeout,cached_at,completed. Devices cached individually via shared cache keys fromget_validated_device_cache_key()
ImportDevicesJobbackground job — imports devices/VMs. Callsbulk_import_devices_shared()for devices andbulk_import_vms()for VMs.job.datakeys: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
Both synchronous and background modes must useget_validated_device_cache_key()fromimport_utils.pyto generate cache keys, ensuring_load_job_results()in the list view can retrieve devices regardless of which mode produced them. Never hardcode cache key formats; always use the helper functions
Use standalone permission helper functions fromimport_utils.pyfor permission checks inside job code:check_user_permissions(user, permissions)→(bool, missing_list)andrequire_permissions(user, permissions, action_description)— raisesPermissionDenied. Background jobs run outside view context and cannot use view mixins
Files:
netbox_librenms_plugin/jobs.py
**/import_validation_helpers.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/import_validation_helpers.py:import_validation_helpers.pymust provide validation state mutation functions:apply_role_to_validation(),apply_cluster_to_validation(),apply_rack_to_validation()for updating validation state when user selects a role/cluster/rack, andremove_validation_issue(),recalculate_validation_status()for maintaining issue list and overall status
import_validation_helpers.pymust provide helper functions:fetch_model_by_id()andextract_device_selections()for reading form data
Files:
netbox_librenms_plugin/import_validation_helpers.py
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust export:validate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (31)
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
📚 Learning: 2026-07-07T22:23:42.118Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.html:5-28
Timestamp: 2026-07-07T22:23:42.118Z
Learning: In netbox-librenms-plugin, do not flag NetBox object detail templates for the config/mapping/rule/pattern-style models when they omit including `extras/inc/custom_fields_panel.html` and `extras/inc/tags_panel.html` (even though these models expose `custom_field_data` and/or `TaggableManager`). This omission is a deliberate, plugin-wide UI convention. If changing this behavior (adding custom-fields/tags panels), require a single coordinated proposal spanning the relevant forms and all detail templates together rather than treating individual missing includes as a regression.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html
📚 Learning: 2026-05-25T21:48:19.264Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html:12-12
Timestamp: 2026-05-25T21:48:19.264Z
Learning: In this plugin’s HTMX form templates, `hx-include` selectors that target toggle preference wrapper `<span>` element IDs (e.g., `#use-sysname-toggle`, `#strip-domain-toggle`, `#auto-create-ipam-toggle`) are intentional. Those wrapper spans contain both the hidden `off` fallback input and the checkbox; HTMX must include the wrapper so the correct value is serialized, including the unchecked/off state. Do not recommend changing `hx-include` to the checkbox IDs with the `-cb` suffix, since it would omit the hidden fallback and break unchecked/off state submission.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/navigation.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/navigation.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/navigation.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/navigation.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-07-01T16:41:50.451Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tables/cables.py:62-76
Timestamp: 2026-07-01T16:41:50.451Z
Learning: When rendering Bootstrap/Tabler badges in this repo (netbox-librenms-plugin), always pair a solid background utility `bg-*` with an explicit text utility that provides appropriate contrast (e.g., `bg-danger` + `text-white`, `bg-warning` + `text-dark`, `bg-purple` + `text-white`). Follow the existing badge contrast convention enforced by `netbox_librenms_plugin/tests/test_badge_contrast.py`. Do not suggest Tabler’s semantic `text-*-fg` token classes (e.g., `text-purple-fg`), since they are not used in this codebase and are not covered by the contrast test.
Applied to files:
netbox_librenms_plugin/navigation.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-05-22T19:37:46.167Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_import_utils.py:1763-1766
Timestamp: 2026-05-22T19:37:46.167Z
Learning: In `validate_device_for_import` (device operations / serial-diff name-resolution logic), preserve the following result contract so the UI/test expectations remain stable:
- If `serial_action` is determined via a serial match AND the existing device has NO OOB/LibreNMS link, set `serial_action` to `"oob_candidate"` and ensure `promote_to_host` is NOT present in the returned dict.
- Populate `promote_to_host` only when the existing device already has an OOB/LibreNMS link (i.e., a host id is available to inherit from); otherwise omit the key.
- Always include `serial_role_choice_available` in the returned dict, defaulting to `False` (baseline) even when other resolution outcomes do not enable it.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-06-25T07:07:59.192Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 114
File: netbox_librenms_plugin/tests/test_oob_sync_review_fixes.py:0-0
Timestamp: 2026-06-25T07:07:59.192Z
Learning: When writing/adjusting tests (and any review/test helper code) that interact with NetBox’s custom User model, do not assume the user model has an `is_staff` field. If you need to check or set user privileges/eligibility, use fields that are known to exist on the NetBox User model (e.g., `is_superuser` and `is_active`) instead. Avoid setting `is_staff` (it can raise Django `FieldError` if that field doesn’t exist on the custom model).
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-07-02T21:46:46.384Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_coverage_bulk_import.py:132-140
Timestamp: 2026-07-02T21:46:46.384Z
Learning: In netbox_librenms_plugin/tests, keep the `_stub_norm_preload` autouse fixture (which patches `netbox_librenms_plugin.import_utils.bulk_import.preload_normalization_rules` to return `{}`) intentionally duplicated per mock-based test class/module. Do NOT hoist it into a global `conftest.py` autouse fixture, because that would apply the patch repo-wide and mask the real `preload_normalization_rules` behavior in real-DB tests that are meant to exercise it (see issue `#90`). If you need this patch, scope it to the specific mock-based tests that should stub normalization rules.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_background_jobs.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-07-02T13:36:15.226Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/interfaces_view.py:226-226
Timestamp: 2026-07-02T13:36:15.226Z
Learning: When reviewing netbox-librenms-plugin view code that handles POSTed `server_key`, treat the plugin-wide convention as intentional: if the posted `server_key` is not present in `LibreNMSAPI.get_available_servers()`, the request should fall back to the currently configured default/active server key (not reject/fail-closed and not treat it as an error for that single view). Do not flag individual instances of this fallback pattern as incorrect “invalid server_key” validation. Any change to fail-closed behavior must be a coordinated cross-cutting change applied uniformly across all affected sites/views, not a one-view patch.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
🪛 ast-grep (0.44.1)
netbox_librenms_plugin/views/imports/actions.py
[error] 817-821: Avoid HTML built in strings
Context: render(
request,
"netbox_librenms_plugin/htmx/bulk_import_collision.html",
{"collisions": collisions},
)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 994-998: Avoid HTML built in strings
Context: render(
request,
"netbox_librenms_plugin/htmx/bulk_import_collision.html",
{"error_message": msg},
)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 1004-1008: Avoid HTML built in strings
Context: render(
request,
"netbox_librenms_plugin/htmx/bulk_import_collision.html",
{"collisions": collisions},
)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 11-11: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 11-11: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 10-10: Tag must be paired, missing: [ ], start tag match failed [
(tag-pair)
[error] 12-12: Tag must be paired, no start tag: [
(tag-pair)
[error] 14-14: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 14-14: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 14-14: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 17-17: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 17-17: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 17-17: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 20-20: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 20-20: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 20-20: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 23-23: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 23-23: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 23-23: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 25-25: Tag must be paired, no start tag: [ ]
(tag-pair)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 11-11: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 11-11: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 10-10: Tag must be paired, missing: [ ], start tag match failed [
(tag-pair)
[error] 12-12: Tag must be paired, no start tag: [
(tag-pair)
[error] 14-14: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 14-14: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 14-14: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 17-17: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 17-17: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 17-17: Tag must be paired, no start tag: [ ]
(tag-pair)
[error] 19-19: Tag must be paired, no start tag: [ ]
(tag-pair)
🪛 markdownlint-cli2 (0.22.1)
docs/usage_tips/interface_mappings.md
[warning] 32-32: Images should have alternate text (alt text)
(MD045, no-alt-text)
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 9, 2026 06:32
f926c98 to
f4a5dc5
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 116-124: Wrap the `api.get_device_info()` call inside
`detect_collisions_for_device_ids` with exception handling so transient
LibreNMS/network errors do not escape the loop. If `get_device_info` raises,
treat that `device_id` the same as a failed `(success, libre_device)` lookup by
appending it to `unresolved_ids` and continuing. Keep the fail-closed behavior
consistent with the surrounding cache lookup and collision-check logic in
`bulk_import.py`.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.html`:
- Around line 9-25: Update the active state handling in _mapping_tabs.html so
each nav-link in the mapping tab list includes aria-current="page" whenever
mapping_tab_active matches that tab’s identifier (for example, the links in the
Interface Mappings, Device Type Mappings, Module Type Mappings, Module Bay
Mappings, and Platform Mappings entries). Keep the existing active class logic
and make the aria-current attribute conditional on the same mapping_tab_active
checks so screen readers announce the current page correctly.
In `@netbox_librenms_plugin/tests/test_coverage_bulk_import.py`:
- Around line 2758-2888: The real-DB coverage in
TestDetectCollisionsForDeviceIds never exercises VM handling, so it can’t catch
regressions where detect_collisions_for_device_ids or validate_device_for_import
mishandles import_as_vm. Add at least one test in this class that uses a VM
fixture/helper instead of make_device, and invoke
detect_collisions_for_device_ids with a cache and sync_options that force the VM
path. Keep the existing collision/unresolved assertions, but make sure the new
case proves VM rows are validated and collision-checked correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6778dff7-de76-4404-8e59-b4d86a2414a2
📒 Files selected for processing (34)
docs/usage_tips/interface_mappings.mddocs/usage_tips/mapping_rules.mdnetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_rules_patterns_navigation.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/modules.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
**/import_utils/__init__.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
import_utils/is a package; the__init__.pymust re-export key functions so callers can usefrom import_utils import ...for functions fromfilters.py,device_operations.py,vm_operations.py,cache.py,permissions.py, andvirtual_chassis.py
Files:
netbox_librenms_plugin/import_utils/__init__.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
**/jobs.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/jobs.py: Background jobs must use NetBox'sJobRunnerbase class (netbox.jobs.JobRunner) for long-running operations like device filtering with VC detection
Use Job UUID (job.job_id) for RQ API endpoints (/api/core/background-tasks/{uuid}/) and Job PK (job.pk) for database endpoints and result loading
RQ status values are:queued,started,finished,stopped,failed(NOTcompleted). Database Job status values are:pending,scheduled,running,completed,failed,errored(NOcancelledstatus exists)
Checkrq_job.is_stoppedorrq_job.is_failedflags in Redis for cancellation detection, not database status
FilterDevicesJobbackground job — filters devices with VC detection.job.datakeys:device_ids,total_processed,filters,server_key,vc_detection_enabled,cache_timeout,cached_at,completed. Devices cached individually via shared cache keys fromget_validated_device_cache_key()
ImportDevicesJobbackground job — imports devices/VMs. Callsbulk_import_devices_shared()for devices andbulk_import_vms()for VMs.job.datakeys: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
Both synchronous and background modes must useget_validated_device_cache_key()fromimport_utils.pyto generate cache keys, ensuring_load_job_results()in the list view can retrieve devices regardless of which mode produced them. Never hardcode cache key formats; always use the helper functions
Use standalone permission helper functions fromimport_utils.pyfor permission checks inside job code:check_user_permissions(user, permissions)→(bool, missing_list)andrequire_permissions(user, permissions, action_description)— raisesPermissionDenied. Background jobs run outside view context and cannot use view mixins
Files:
netbox_librenms_plugin/jobs.py
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments live in
templates/netbox_librenms_plugin/htmx/including:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
**/import_validation_helpers.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/import_validation_helpers.py:import_validation_helpers.pymust provide validation state mutation functions:apply_role_to_validation(),apply_cluster_to_validation(),apply_rack_to_validation()for updating validation state when user selects a role/cluster/rack, andremove_validation_issue(),recalculate_validation_status()for maintaining issue list and overall status
import_validation_helpers.pymust provide helper functions:fetch_model_by_id()andextract_device_selections()for reading form data
Files:
netbox_librenms_plugin/import_validation_helpers.py
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust export:validate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
🧠 Learnings (26)
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_mapping_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-07-07T22:23:42.118Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.html:5-28
Timestamp: 2026-07-07T22:23:42.118Z
Learning: In netbox-librenms-plugin, do not flag NetBox object detail templates for the config/mapping/rule/pattern-style models when they omit including `extras/inc/custom_fields_panel.html` and `extras/inc/tags_panel.html` (even though these models expose `custom_field_data` and/or `TaggableManager`). This omission is a deliberate, plugin-wide UI convention. If changing this behavior (adding custom-fields/tags panels), require a single coordinated proposal spanning the relevant forms and all detail templates together rather than treating individual missing includes as a regression.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.html
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-07-01T16:41:50.451Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tables/cables.py:62-76
Timestamp: 2026-07-01T16:41:50.451Z
Learning: When rendering Bootstrap/Tabler badges in this repo (netbox-librenms-plugin), always pair a solid background utility `bg-*` with an explicit text utility that provides appropriate contrast (e.g., `bg-danger` + `text-white`, `bg-warning` + `text-dark`, `bg-purple` + `text-white`). Follow the existing badge contrast convention enforced by `netbox_librenms_plugin/tests/test_badge_contrast.py`. Do not suggest Tabler’s semantic `text-*-fg` token classes (e.g., `text-purple-fg`), since they are not used in this codebase and are not covered by the contrast test.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-05-22T19:37:46.167Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_import_utils.py:1763-1766
Timestamp: 2026-05-22T19:37:46.167Z
Learning: In `validate_device_for_import` (device operations / serial-diff name-resolution logic), preserve the following result contract so the UI/test expectations remain stable:
- If `serial_action` is determined via a serial match AND the existing device has NO OOB/LibreNMS link, set `serial_action` to `"oob_candidate"` and ensure `promote_to_host` is NOT present in the returned dict.
- Populate `promote_to_host` only when the existing device already has an OOB/LibreNMS link (i.e., a host id is available to inherit from); otherwise omit the key.
- Always include `serial_role_choice_available` in the returned dict, defaulting to `False` (baseline) even when other resolution outcomes do not enable it.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-05-25T21:48:19.264Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html:12-12
Timestamp: 2026-05-25T21:48:19.264Z
Learning: In this plugin’s HTMX form templates, `hx-include` selectors that target toggle preference wrapper `<span>` element IDs (e.g., `#use-sysname-toggle`, `#strip-domain-toggle`, `#auto-create-ipam-toggle`) are intentional. Those wrapper spans contain both the hidden `off` fallback input and the checkbox; HTMX must include the wrapper so the correct value is serialized, including the unchecked/off state. Do not recommend changing `hx-include` to the checkbox IDs with the `-cb` suffix, since it would omit the hidden fallback and break unchecked/off state submission.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-25T07:07:59.192Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 114
File: netbox_librenms_plugin/tests/test_oob_sync_review_fixes.py:0-0
Timestamp: 2026-06-25T07:07:59.192Z
Learning: When writing/adjusting tests (and any review/test helper code) that interact with NetBox’s custom User model, do not assume the user model has an `is_staff` field. If you need to check or set user privileges/eligibility, use fields that are known to exist on the NetBox User model (e.g., `is_superuser` and `is_active`) instead. Avoid setting `is_staff` (it can raise Django `FieldError` if that field doesn’t exist on the custom model).
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-07-02T21:46:46.384Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_coverage_bulk_import.py:132-140
Timestamp: 2026-07-02T21:46:46.384Z
Learning: In netbox_librenms_plugin/tests, keep the `_stub_norm_preload` autouse fixture (which patches `netbox_librenms_plugin.import_utils.bulk_import.preload_normalization_rules` to return `{}`) intentionally duplicated per mock-based test class/module. Do NOT hoist it into a global `conftest.py` autouse fixture, because that would apply the patch repo-wide and mask the real `preload_normalization_rules` behavior in real-DB tests that are meant to exercise it (see issue `#90`). If you need this patch, scope it to the specific mock-based tests that should stub normalization rules.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
🔇 Additional comments (27)
docs/usage_tips/interface_mappings.md (1)
32-32: Add alt text to the screenshot. The image still has an empty alt field, so the accessibility/MD045 issue remains.netbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.html (1)
3-6: LGTM!Also applies to: 19-24
netbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.html (1)
3-6: LGTM!Also applies to: 19-24
netbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.html (1)
3-6: LGTM!Also applies to: 19-24
docs/usage_tips/mapping_rules.md (1)
5-5: LGTM!netbox_librenms_plugin/import_utils/__init__.py (1)
17-55: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.html (1)
3-7: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.html (1)
3-7: LGTM!netbox_librenms_plugin/tests/test_coverage_bulk_import.py (1)
968-1054: LGTM!netbox_librenms_plugin/import_validation_helpers.py (1)
12-43: LGTM!netbox_librenms_plugin/import_utils/device_operations.py (2)
887-893: LGTM!Also applies to: 931-931, 1027-1038
1075-1078: 🎯 Functional CorrectnessAdd a regression test for the primary-IP ambiguity early return. This branch skips the later site/device-type/role validation path and is easy to miss in this function.
netbox_librenms_plugin/import_utils/bulk_import.py (2)
475-502: LGTM!Also applies to: 587-599
77-141: 🎯 Functional CorrectnessConfirm the caller split before threading
import_as_vmhere. This helper is device-scoped (device_ids,bulk_import_devices_shared); whether any VM import path reaches it is still unclear from the available context.netbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.html (1)
3-7: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.html (1)
3-7: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.html (1)
3-7: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html (1)
1-80: LGTM!netbox_librenms_plugin/navigation.py (1)
1-3: LGTM!Also applies to: 39-60
netbox_librenms_plugin/jobs.py (1)
189-189: Confirm thedetect_collisions_for_device_idscontract
jobs.pydepends ondetect_collisions_for_device_idsbeing re-exported fromimport_utilsand returning a 2-tuple(collisions, unresolved)for the unpacking here.netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.html (1)
1-19: LGTM!netbox_librenms_plugin/tests/test_background_jobs.py (1)
375-396: LGTM!Also applies to: 434-450, 485-513, 523-527, 615-632, 708-722, 749-793, 795-837, 839-871
netbox_librenms_plugin/tests/test_cable_verify.py (1)
380-391: LGTM!Also applies to: 500-505
netbox_librenms_plugin/import_utils/collisions.py (2)
1-225: LGTM!
18-18: 🗄️ Data Integrity & IntegrationCheck
import_utils/__init__.pyexportsdetect_collisions_for_device_idsIf callers import from
netbox_librenms_plugin.import_utils, this helper needs to be re-exported there; includedetect_bulk_collisionstoo if it is part of the public API.netbox_librenms_plugin/tests/test_collisions.py (1)
1-429: LGTM!netbox_librenms_plugin/tests/test_import_validation_helpers.py (1)
388-506: LGTM!Also applies to: 525-654, 701-721
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 9, 2026 23:38
f4a5dc5 to
4a952ce
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
July 10, 2026 07:59
4a952ce to
eedcec7
Compare
…ct literal The single-string assertion coupled the switcher tests to exact attribute order/spacing — inserting e.g. role="tab" between attributes would fail them with the pill still correctly active. Extract the aria-current pill and assert the active class and href independently, keeping the exactly-one aria-current="page" contract.
The plugin registers a top-level PluginMenu labelled LibreNMS; the page still directed readers to a 'Netbox Librenms Plugin' entry under the generic Plugins section.
marcinpsk
force-pushed
the
feat/nav-consolidation
branch
from
August 9, 2026 07:00
14ea70c to
bad417a
Compare
marcinpsk
commented
Aug 9, 2026
Owner
Author
|
@coderabbitai review |
Rate Limit Exceeded
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Aug 12, 2026
Closed
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
1 participant
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
Collapse the plugin sidebar's nine per-model Mappings items (five type mappings + four rule/pattern engines) into two single entries — Mappings and Rules & Patterns. The individual object lists now cross-link through a switcher-pills row rendered above the native Results/Filters tabs, so each tab is the model's real
ObjectListViewpage (not an embedded pane) and keeps the full generic feature set (filters, bulk ops, YAML import/export, changelog). The active pill is announced to screen readers witharia-current="page".feat(nav): collapse the mapping/rule menus into the two tabbed entries; move the per-model menu buttons off the sidebar (each list page already carries its own Add/Import controls).fix(nav): announce the active switcher pill witharia-current="page"(Bootstrap nav-pills a11y) on both switchers.test(nav): assert the active pill by attributes (class + href + exactly-onearia-current="page"), decoupled from exact attribute order.Motivation / Problem
Nine sidebar items crowded the plugin menu. Grouping them into two entries with an in-page switcher keeps every list fully functional while decluttering the nav. Convention going forward (enforced by the navigation tests): a branch that introduces a new rule/pattern model adds its switcher tab in the same commit that adds the model —
feat/parent-child-interfaces(Port Stack LAG Patterns) andfeat/serial-ports(Serial Sensor Types) already follow it up-stack.Scope of Change
How Was This Tested?
test_rules_patterns_navigation.py, asserting each tab renders the real list page and that exactly onearia-current="page"pill is announced per page. Red → green on the a11y change.Manual Test Steps
aria-current="page".ObjectListView(filters, bulk operations, import/export, changelog all present).Risk Assessment
Backwards Compatibility
Other Notes
Stacked on
feat/bulk-import(#115). The only delta this PR adds over that base is the navigation consolidation described above (navigation.py, the_mapping_tabs.html/_rules_patterns_tabs.htmlswitcher includes, the tab row added to the eight list templates,test_rules_patterns_navigation.py, and the two docs menu-path updates).Summary by CodeRabbit
New Features
Bug Fixes
Documentation