feat: UI improvements#299
Merged
10 commits merged intoJun 1, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds platform-mapping HTMX forms and typeaheads to the device import validation UI, a backend AddPlatformMappingView that creates/updates PlatformMapping under transactional locking, refactors validation templates to use shared mapping partials (and a platform-manage icon), expands mapping rules and Module Sync documentation, and updates import-setting markup and JS including tighter HTMX modal-dismiss behaviour. Sequence Diagram(s)sequenceDiagram
participant User
participant ImportModal
participant HTMXPartial as _platform/_dt partial
participant AddPlatformView as AddPlatformMappingView
participant ORM as PlatformMapping ORM
participant Cache
participant UIUpdate as HTMX OOB Response
User->>ImportModal: Open validation modal
ImportModal->>HTMXPartial: Render mapping form (typeahead)
User->>HTMXPartial: Submit platform_id for LibreNMS OS
HTMXPartial->>AddPlatformView: POST via HTMX
AddPlatformView->>ORM: Load device, query existing mappings
AddPlatformView->>ORM: Begin transaction, select_for_update
ORM->>ORM: Create or update PlatformMapping (or raise IntegrityError)
AddPlatformView->>Cache: Invalidate per-device import cache
AddPlatformView->>UIUpdate: Return OOB swaps (modal + device row)
UIUpdate->>ImportModal: Update modal content and device table row
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 31, 2026
Existing devices whose LibreNMS OS has no PlatformMapping previously only got the 'Create Platform' path, nudging users toward duplicate Platform records. Mirror the device-type flow: also offer the platform-mapping form — preselected to the device's current platform when it has one, or unselected when it doesn't — so the LibreNMS OS can be mapped to an existing platform instead. Addresses CodeRabbit feedback surfaced on PR #84 (the file belongs to this branch / PR bonzo81#299).
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
May 31, 2026
… locking AddPlatformMappingView now rejects an ambiguous OS string up front, and inside the transaction materialises the candidate rows with select_for_update()[:2] instead of count(). count() silently drops the FOR UPDATE clause, so the rows were never actually locked before the create/update decision (race window). Addresses CodeRabbit feedback on PR bonzo81#303; the platform-mapping logic belongs to this branch / PR bonzo81#299.
Owner
|
Nice UI/UX updates @marcinpsk. Tested in the UI but I'll kick of a Copilot review, and should be good to merge after that. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the LibreNMS device import/validation UX by extracting inline mapping forms into reusable HTMX partials, adding an inline “Add Platform Mapping” flow from the validation modal, and fixing several import toggle + nested modal behaviors.
Changes:
- Added
AddPlatformMappingViewand wired it into the plugin’s view exports and URLconf to allow creating/updatingPlatformMappingdirectly from the import modal. - Refactored validation modal templates to use new reusable partials for device-type mapping and platform mapping/management UI.
- Fixed import toggle submission behavior (unchecked values) and corrected nested-modal dismiss handling; added a test to prevent multi-line
{# #}Django comments from rendering as literal text.
Reviewed changes
Copilot reviewed 17 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| netbox_librenms_plugin/views/imports/actions.py | Adds AddPlatformMappingView and extends platform modal context for the combined “manage platform” flow. |
| netbox_librenms_plugin/views/init.py | Exports AddPlatformMappingView for URL registration/import convenience. |
| netbox_librenms_plugin/urls.py | Registers the new device-import/add-platform-mapping/<int:device_id>/ endpoint. |
| netbox_librenms_plugin/tests/test_template_comments.py | Adds a static test to detect multi-line {# #} comments in Django templates. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html | Ensures unchecked toggles submit values and updates toggle element IDs to match JS. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html | Refactors mapping UI into includes and introduces compact platform “manage” icon usage. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.html | Extends the modal to support mapping LibreNMS OS to an existing platform before creating a new one. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html | New reusable “Add Platform Mapping” typeahead form partial. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_manage_icon.html | New compact UI control to open the combined platform manage/create modal. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html | New reusable “Add Device Type Mapping” typeahead form partial. |
| netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js | Updates toggle ID references and fixes dismiss handling for nested modals. |
| docs/usage_tips/README.md | Updates usage tips index to include mapping rules and module sync docs. |
| docs/usage_tips/module_sync.md | Adds new end-user documentation for the Module Sync workflow. |
| docs/usage_tips/mapping_rules.md | Adds new end-user documentation for mapping rule types and YAML formats. |
| docs/README.md | Highlights module/inventory sync and links to the new docs pages. |
| docs/librenms_import/validation.md | Updates validation docs to reference platform/device-type mappings and the platform modal flow. |
| docs/feature_list.md | Expands feature list to include mapping rules and module/inventory sync. |
marcinpsk
added a commit
to marcinpsk/netbox-librenms-plugin
that referenced
this pull request
Jun 1, 2026
- _dt_mapping_form / _platform_mapping_form: post the server_key hidden field
(inherited from the render context) so Add{DeviceType,Platform}MappingView run
the post-save re-validation + cache invalidation against the same LibreNMS
server the modal was rendered for, not the default — fixes wrong-server
context in multi-server setups.
- AddPlatformMappingView: logger.exception instead of logger.warning in the save
failure handler, to preserve the traceback and match AddDeviceTypeMappingView.
Addresses Copilot review on bonzo81#299.
Owner
|
@marcinpsk similar to #303, looks like some unrelated files in this PR from the development process split |
- Extract device type mapping form into reusable partial (_dt_mapping_form.html) - Extract platform mapping form into reusable partial (_platform_mapping_form.html) - Add AddPlatformMappingView for creating PlatformMapping from import modal - Register add_platform_mapping URL - device_validation_details.html: replace 100-line inline DT form with include; add DT mapping option in mismatch branch; add no-mapping elif with warning; add platform mapping form in platform row - librenms_sync_base.html: fix VM-without-cluster breadcrumb; add object_model_name guard on platform modal; rename _create_platform_url -> create_platform_url - librenms_import.html: wrap toggles with span+hidden-input so unchecked values submit; rename checkbox IDs to -cb suffix - librenms_import.js: rename toggle IDs to match -cb suffix; fix nested modal dismiss so inner modals (e.g. Promote-to-host picker) are not broken
… reachable
Outer {% if validation.existing_device and ... %} at line 293 was missing
its {% endif %}, breaking template compilation. The new-import branch's
platform mapping include was also guarded on sync_info, which is only set
when an existing device matches — so the form never rendered for new
imports. Drop the sync_info clause and the unreachable preselect_platform.
The upfront permission check at line 1849-1854 selects "add" or "change" based on whether a mapping exists, but the get_or_create that follows could race with a concurrent delete: a caller with only "change" permission could end up creating a new row. Mirror the established AddDeviceTypeMappingView pattern — lock the row with select_for_update, re-derive the required permission from the locked state, and surface IntegrityError on concurrent inserts.
Mirror the sibling AddDeviceTypeMappingView style for consistent HTMX toast behavior on validation errors, and reject the LibreNMS "-" placeholder OS that previously slipped past the empty-string check.
Existing devices whose LibreNMS OS has no PlatformMapping previously only got the 'Create Platform' path, nudging users toward duplicate Platform records. Mirror the device-type flow: also offer the platform-mapping form — preselected to the device's current platform when it has one, or unselected when it doesn't — so the LibreNMS OS can be mapped to an existing platform instead. Addresses CodeRabbit feedback surfaced on PR #84 (the file belongs to this branch / PR bonzo81#299).
… locking AddPlatformMappingView now rejects an ambiguous OS string up front, and inside the transaction materialises the candidate rows with select_for_update()[:2] instead of count(). count() silently drops the FOR UPDATE clause, so the rows were never actually locked before the create/update decision (race window). Addresses CodeRabbit feedback on PR bonzo81#303; the platform-mapping logic belongs to this branch / PR bonzo81#299.
…ed modal The platform cell in the import validation modal had become cluttered (inline mapping typeahead + Create Platform button + sync button all at once). Replace the inline forms/buttons with a single pencil icon that opens one modal offering BOTH options: - map the LibreNMS OS to an existing platform (typeahead), and - create a new platform. The icon also appears where a platform is already resolved (direct match or via mapping), so the mapping can be changed compactly. The combined modal reuses the existing create-platform modal plus the _platform_mapping_form partial, guarded so the device-sync page (create-only, no libre_device) is unaffected. - _platform_manage_icon.html: the compact pencil-icon trigger - create_platform_modal.html: adds a 'map to existing' section when libre_device is present; neutral alert wording - CreatePlatformFromImportView.get: passes libre_device + current_platform - device_validation_details.html: platform cell rewritten to use the icon; inline _platform_mapping_form includes + Create Platform buttons removed
… leaked as text)
Django's {# #} comment cannot span multiple lines, so the multi-line header
comment rendered literally in the platform cell. Switch to a {% comment %}
block.
Static scan of all plugin .html templates flagging any line with an unbalanced
{# / #} count (the signature of a multi-line {# #}, which Django renders as
literal text). Would have caught the _platform_manage_icon regression that
get_template()/template-compile checks missed.
- _dt_mapping_form / _platform_mapping_form: post the server_key hidden field
(inherited from the render context) so Add{DeviceType,Platform}MappingView run
the post-save re-validation + cache invalidation against the same LibreNMS
server the modal was rendered for, not the default — fixes wrong-server
context in multi-server setups.
- AddPlatformMappingView: logger.exception instead of logger.warning in the save
failure handler, to preserve the traceback and match AddDeviceTypeMappingView.
Addresses Copilot review on bonzo81#299.
marcinpsk
force-pushed
the
feat/ui-improvements
branch
from
ba2ea09 to
abf769b
Compare
Open
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
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.
follows #288
Summary
Improves the import / validation UI. The device-type and platform mapping forms in the validation modal are extracted into reusable partials, a new Add Platform Mapping flow lets users create a
PlatformMappingdirectly from the import modal, and several template/JS bugs around the import toggles and nested modals are fixed.Motivation / Problem
PlatformMappingfrom the import validation modal (AddPlatformMappingView) so users can resolve unmatched platforms without leaving the import flow.device_validation_details.html; extract them into reusable partials.{% endif %}broke template compilation and the platform mapping form never rendered for new imports.Scope of Change
Specifically:
AddPlatformMappingViewadded toviews/imports/actions.py, registered inurls.pyand exported fromviews/__init__.py._dt_mapping_form.htmland_platform_mapping_form.htmlpartials;device_validation_details.html,librenms_import.html,librenms_sync_base.htmlupdated.librenms_import.jstoggle ID renames (-cbsuffix) and nested-modal dismiss fix.How Was This Tested?
AddDeviceTypeMappingViewpattern.Manual Test Steps
-OS rejected).Risk Assessment
PlatformMapping;AddPlatformMappingViewusesselect_for_updateto re-derive the required permission from the locked row and surfacesIntegrityErroron concurrent inserts, closing a TOCTOU window.Backwards Compatibility
Other Notes
AddPlatformMappingViewdeliberately mirrorsAddDeviceTypeMappingView(permission handling,_htmx_error_response,-placeholder OS rejection) for consistency — worth reviewing the two side by side.