Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The task summary counters in IpScan are inconsistent with filtered/paginated results, and filtering forms currently lack non-JS submit/reset fallbacks (accessibility/progressive-enhancement regression).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enhances the Zabbix IPAM module UI/UX and backend controllers by introducing reusable pagination + time formatting helpers, adding per-page selection and auto-filtering, and aligning scheduled scan behavior with a cron-driven interval model. It also bumps module versioning/documentation to reflect these changes.
Changes:
- Add
TimeFormatterandViewPaginationhelpers and apply them across IPAM list pages (scan tasks / ranges / IP details). - Introduce pagination + per-page selection (20/50/100/200) in
IpManager,IpScan, andIpDetailcontrollers and views. - Update cron scheduling semantics to scan all enabled ranges per cron run; refresh UI styling and documentation/version metadata.
File summaries
| File | Description |
|---|---|
| zabbix_ipam/views/ip.scan.php | Task list UI updated: row numbering, formatted timestamps, pagination, auto-filtering. |
| zabbix_ipam/views/ip.manager.php | Range list UI updated: split filters, row numbering, last-scan time, pagination, auto-filtering. |
| zabbix_ipam/views/ip.detail.php | IP detail list UI updated: row numbering, status-updated time, pagination, auto-filtering. |
| zabbix_ipam/README.md | Expanded documentation (features, compatibility, install, cron behavior, page descriptions). |
| zabbix_ipam/manifest.json | Module metadata updated (name/author/version). |
| zabbix_ipam/lib/ViewPagination.php | New shared pagination renderer (prev/next + per-page selector). |
| zabbix_ipam/lib/TimeFormatter.php | New shared timestamp display helper using Zabbix user timezone when available. |
| zabbix_ipam/lib/TaskManager.php | due() behavior changed to schedule all enabled ranges each cron invocation. |
| zabbix_ipam/lib/LanguageManager.php | New/updated i18n strings for pagination, new columns, cron schedule wording. |
| zabbix_ipam/lib/HostMatcher.php | Host link updated to point to “latest data” view rather than edit page. |
| zabbix_ipam/cli/scan_cron.php | Avoid running pending tasks that were already dispatched (checks dispatched_at). |
| zabbix_ipam/assets/js/ipam.js.php | Remove scan_interval handling, add auto-filter submit behavior, prevent default on count link. |
| zabbix_ipam/assets/css/ipam-responsive.css | Responsive/layout + button styling changes; pagination and modal sizing rules. |
| zabbix_ipam/actions/IpScan.php | Adds pagination/per-page and summary data for task list. |
| zabbix_ipam/actions/IpManager.php | Adds pagination/per-page and new filter model (enabled_status/scan_status). |
| zabbix_ipam/actions/IpDetail.php | Adds per-page selection and exposes status update timestamp per row. |
| zabbix_ipam/actions/IpAjax.php | Removes scan_interval input handling to match cron-driven schedule model. |
Review details
- Files reviewed: 17/20 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+50
to
+67
| $tasks = []; | ||
| $summary = ['pending' => 0, 'running' => 0, 'completed' => 0, 'failed' => 0]; | ||
| foreach ($storage->tasks() as $task) { | ||
| if (isset($summary[$task['status']])) { | ||
| $summary[$task['status']]++; | ||
| } | ||
| $task['range_name'] = $range_names[$task['range_id']] ?? $task['range_id']; | ||
| if ($task_id !== '' && $task['id'] !== $task_id) { | ||
| continue; | ||
| } | ||
| if ($status !== 'all' && $task['status'] !== $status) { | ||
| continue; | ||
| } | ||
| if ($search !== '' && strpos(mb_strtolower($task['id'].' '.$task['range_name']), $search) === false) { | ||
| continue; | ||
| } | ||
| $tasks[] = $task; | ||
| } |
Comment on lines
+62
to
+66
| .'<form class="ipam-toolbar ip-detail-toolbar js-auto-filter" method="get"><input type="hidden" name="action" value="ip.detail"><input type="hidden" name="per_page" value="'.$e($data['pagination']['per_page']).'">' | ||
| .'<label><span>'.$t('Search').'</span><input name="search" value="'.$e($filters['search']).'" placeholder="'.$t('Search IP, range, or host name').'"></label>' | ||
| .'<label><span>'.$t('IP range').'</span><select name="rangeid">'.$range_options.'</select></label>' | ||
| .'<label><span>'.$t('IP status').'</span><select name="status">'.$status_options.'</select></label>' | ||
| .'<label><span>'.$t('Host association').'</span><select name="associated">'.$association_options.'</select></label>' | ||
| .'<button class="btn-alt">'.$t('Filter').'</button><a class="btn-link" href="zabbix.php?action=ip.detail">'.$t('Reset').'</a></form>' | ||
| .'<label><span>'.$t('Host association').'</span><select name="associated">'.$association_options.'</select></label></form>' |
Comment on lines
+71
to
+75
| .'<form class="ipam-toolbar js-auto-filter" method="get"><input type="hidden" name="action" value="ip.manager"><input type="hidden" name="per_page" value="'.$e($data['pagination']['per_page']).'">' | ||
| .'<label><span>'.$t('Search').'</span><input name="search" value="'.$e($data['filters']['search']).'" placeholder="'.$t('Search name or CIDR').'"></label>' | ||
| .'<label><span>'.$t('IP range').'</span><select name="rangeid">'.$range_options.'</select></label>' | ||
| .'<label><span>'.$t('Status').'</span><select name="status">'.$status_options.'</select></label>' | ||
| .'<button class="btn-alt">'.$t('Filter').'</button><a class="btn-link" href="zabbix.php?action=ip.manager">'.$t('Reset').'</a></form>' | ||
| .'<div class="ipam-table-wrap"><table class="ipam-table ipam-range-table"><thead><tr><th>'.$t('IP range').'</th><th>'.$t('Alive IPs / Total').'</th><th>'.$t('Actions').'</th></tr></thead>' | ||
| .'<tbody>'.($rows ?: '<tr><td colspan="3" class="ipam-empty">'.$t('No IP ranges found.').'</td></tr>').'</tbody></table></div></div>'; | ||
| .'<label><span>'.$t('Enabled status').'</span><select name="enabled_status">'.$enabled_options.'</select></label>' | ||
| .'<label><span>'.$t('Scan status').'</span><select name="scan_status">'.$scan_options.'</select></label></form>' |
Comment on lines
+64
to
+66
| .'<form class="ipam-toolbar js-auto-filter" method="get"><input type="hidden" name="action" value="ip.scan"><input type="hidden" name="per_page" value="'.$e($data['pagination']['per_page']).'">' | ||
| .'<label><span>'.$t('Search').'</span><input name="search" value="'.$e($data['filters']['search']).'" placeholder="'.$t('Search task ID or range name').'"></label>' | ||
| .'<label><span>'.$t('Status').'</span><select name="status">'.$status_options.'</select></label><button class="btn-alt">'.$t('Filter').'</button><a class="btn-link" href="zabbix.php?action=ip.scan">'.$t('Reset').'</a></form>' | ||
| .'<div class="ipam-table-wrap"><table class="ipam-table ipam-task-table"><thead><tr><th>'.$t('Task / IP range').'</th><th>'.$t('Status').'</th><th>'.$t('Progress').'</th><th>'.$t('Alive').'</th><th>'.$t('Shards').'</th><th>'.$t('Created').'</th><th>'.$t('Actions').'</th></tr></thead>' | ||
| .'<tbody>'.($rows ?: '<tr><td colspan="7" class="ipam-empty">'.$t('No scan tasks found.').'</td></tr>').'</tbody></table></div></div>' | ||
| .'<div class="ipam-toast" id="ipam-toast" hidden></div><script src="modules/zabbix_ipam/assets/js/ipam.js.php?v=1.2.0"></script>'; | ||
| .'<label><span>'.$t('Status').'</span><select name="status">'.$status_options.'</select></label></form>' |
| "author": "火星小刘", | ||
| "url": "https://github.com/X-Mars/zabbix_modules", | ||
| "version": "1.2.0", | ||
| "version": "1.5.0", |
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.
No description provided.