[fix] Highlighting IP in go to IP instead of deleting previous rows #73

[pranshustuff]

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Fixes #73

Please describe these changes.

Instead of deleting previous rows when fetching IP in search, it now scrolls to the IP till it is visible on screen and highlights it.

Screenshot

simplescreenrecorder-2025-10-02_12.24.33.mp4

[pranshustuff]

I've passed the QA checks for subnet.js. It was showing format differences for jstree ad plotly but those files I haven't changed.

[nemesifier]

Thanks @pranshustuff, this is a sensible UX fix for #73: when the target IP is already rendered, scroll to and highlight it rather than clearing and refetching the visualization.

A few notes:

1. Highlight vs scroll timing. The scroll animation is 500ms and the highlight is cleared after 500ms, so on a long scroll the yellow flash may fade before the user's eye lands on it. Consider clearing the highlight after the scroll completes (animation callback) or using a slightly longer highlight.
2. Only used IPs get an id. The id="addr_<id>" is added only inside if (addr.used), so "go to" an unused-but-rendered IP will not find a target and falls through to the refetch path. That may be acceptable, just confirm it is intentional.
3. ID sanitization. Ensure normalizeIP produces an id safe for a DOM id/selector for IPv6 too (no colons), otherwise `` will break for v6 addresses.
4. Prefer toggling a CSS class (e.g. .highlight) over inline css("background-color", "yellow"), so the color respects the admin theme (including dark mode).

Minor stuff; the core behavior is a good improvement. A small QUnit/JS test or at least manual confirmation for both an on-page hit and an off-page (refetch) case would be nice.

[nemesifier]

Thanks @pranshustuff, this is a sensible UX fix for #73: when the target IP is already rendered, scroll to and highlight it rather than clearing and refetching the visualization.

A few notes:

1. Highlight vs scroll timing. The scroll animation is 500ms and the highlight is cleared after 500ms, so on a long scroll the yellow flash may fade before the user's eye lands on it. Consider clearing the highlight after the scroll completes (animation callback) or using a slightly longer highlight.
2. Only used IPs get an id. The id="addr_<id>" is added only inside if (addr.used), so "go to" an unused-but-rendered IP will not find a target and falls through to the refetch path. That may be acceptable, just confirm it is intentional.
3. ID sanitization. Ensure normalizeIP produces an id safe for a DOM id/selector for IPv6 too (no colons), otherwise $("#addr_" + id) will break for v6 addresses.
4. Prefer toggling a CSS class (e.g. .highlight) over inline css("background-color", "yellow"), so the color respects the admin theme (including dark mode).

Minor stuff; the core behavior is a good improvement. A small QUnit/JS test or at least manual confirmation for both an on-page hit and an off-page (refetch) case would be nice.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request enhances the subnet IP lookup feature by adding stable DOM IDs to used address elements and redesigning the "Go To IP" navigation flow. Used IP addresses now receive an id="addr_<normalized>" attribute during rendering. The goTo function is refactored to first check whether the target IP is already visible in the current DOM; if present, it scrolls to and highlights the element. If absent, it resets pagination and search state, clears the list, sets the new search page URL, and triggers a fetch to load results containing the target. Invalid input now returns early after displaying an error message. A minor CSS formatting change removes an extra blank line between rule blocks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Bug Fixes	❌ Error	PR partially fixes bug but reintroduces it when target IP not on page. Pagination still clears on re-fetch, preventing scroll-back. No regression or Selenium tests included.	Add regression tests for both visible and non-visible IP cases; add Selenium test for UI interaction verification to ensure scrolling works without clearing pagination.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses the required [fix] prefix, is descriptive and directly addresses the linked issue #73 about the go to IP feature behavior.
Description check	✅ Passed	The description follows the template with completed key sections: checklist (2/4 items checked), reference to issue #73, clear change description, and a screenshot demonstrating the fix.
Linked Issues check	✅ Passed	The code changes address issue #73 by implementing the ability to scroll to and highlight the target IP without deleting previous rows, restoring full navigation capability.
Out of Scope Changes check	✅ Passed	All changes are scoped to the linked issue #73: CSS whitespace adjustments and subnet.js modifications to implement the new scroll-and-highlight behavior.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@openwisp_ipam/static/openwisp-ipam/js/subnet.js`:
- Around line 140-147: The code resets the UI and pagination state (calls
$("`#subnet-visual`").empty() and clears fetchedPages/lastRenderedPage) in the
go-to-IP fallback, which loses navigation history; instead do not clear the
existing DOM or pagination caches — only set searchQuery and nextPageUrl
("/api/v1/ipam/subnet/"+current_subnet+"/hosts/?start="+searchQuery") and then
drive the existing fetch/scroll loop to request additional pages until the
target IP is rendered and visible; remove the $("`#subnet-visual`").empty(), do
not reset fetchedPages or lastRenderedPage, and ensure busy is managed (set true
when starting the fetch loop and false when done) so the current fetch/scroll
machinery will append pages and allow scrolling back.

🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 73766b1a-f84a-484b-9368-c91d576bc09c

📥 Commits

Reviewing files that changed from the base of the PR and between 922b9e1 and 1a2e3f8.

📒 Files selected for processing (2)

• openwisp_ipam/static/openwisp-ipam/css/admin.css
• openwisp_ipam/static/openwisp-ipam/js/subnet.js

💤 Files with no reviewable changes (1)

• openwisp_ipam/static/openwisp-ipam/css/admin.css

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.2.0

🔇 Additional comments (1)

openwisp_ipam/static/openwisp-ipam/js/subnet.js (1)

59-70: LGTM!

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Preserve pagination state instead of clearing it in go-to fallback

Line 144 through Line 147 still wipe the rendered list and cached pages. That discards navigation history and reintroduces the “can’t scroll back after go to IP” behavior for addresses not already in DOM. Keep existing rows/state and fetch/scroll until the target becomes visible instead of resetting fetchedPages/lastRenderedPage.

🤖 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 `@openwisp_ipam/static/openwisp-ipam/js/subnet.js` around lines 140 - 147, The
code resets the UI and pagination state (calls $("`#subnet-visual`").empty() and
clears fetchedPages/lastRenderedPage) in the go-to-IP fallback, which loses
navigation history; instead do not clear the existing DOM or pagination caches —
only set searchQuery and nextPageUrl
("/api/v1/ipam/subnet/"+current_subnet+"/hosts/?start="+searchQuery") and then
drive the existing fetch/scroll loop to request additional pages until the
target IP is rendered and visible; remove the $("`#subnet-visual`").empty(), do
not reset fetchedPages or lastRenderedPage, and ensure busy is managed (set true
when starting the fetch loop and false when done) so the current fetch/scroll
machinery will append pages and allow scrolling back.