Refactoring - Migrate JS functions to functions.js #242
Conversation
This feature allows users to group messeges based on similar text and host This is useful for when a host has been sending repeted alarms of the same type
Move JS functions to functions.js file - Easier and cleaner
There was a problem hiding this comment.
Pull request overview
This pull request refactors the syslog plugin's frontend JavaScript code by migrating inline JavaScript to a centralized js/functions.js file, adds new GitHub agent templates for development workflows, and introduces a message grouping feature for the syslog viewer to improve UI/UX when dealing with duplicate messages.
Key Changes:
- Centralized JavaScript functions in
js/functions.jswith new initialization functions (initSyslogMain,initSyslogAlerts,initSyslogReports,initSyslogRemoval,initSyslogStats) to replace inline scripts - Added message grouping functionality in syslog viewer with expandable/collapsible detail rows for viewing individual occurrences of grouped messages
- Created three new GitHub agent templates for Code Quality, PHP Development, and MySQL/MariaDB database administration to assist with development workflows
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
js/functions.js |
New file containing all refactored JavaScript functions from inline scripts, organized by functional area (main view, alerts, reports, removal, statistics) |
functions.php |
Added syslog_include_js() helper function to load the new JavaScript file; modified syslog_row_color() to return severity class and add syslog-detail-row class |
syslog.php |
Added syslog_include_js() call; implemented message grouping feature with GROUP BY queries, COUNT aggregation, and expandable detail rows; added grouping filter control; replaced inline JavaScript with initSyslogMain() and initSyslogStats() calls |
syslog_alerts.php |
Added syslog_include_js() calls in all action cases; replaced inline JavaScript with initSyslogAlerts() call |
syslog_reports.php |
Added syslog_include_js() calls in all action cases; replaced inline JavaScript with initSyslogReports() call |
syslog_removal.php |
Added syslog_include_js() calls in all action cases; replaced inline JavaScript with initSyslogRemoval() call |
syslog.css |
Added CSS styles for message grouping toggle icons and detail row hover effects |
.gitignore |
Removed .git* pattern to allow addition of .github directory files |
.github/ISSUE_TEMPLATE/copilot-instructions.md |
Updated with clarification on syslog message handling and added links to Cacti documentation and main repository |
.github/ISSUE_TEMPLATE/agents/code-quality.agent.md |
New agent template for Code Quality Specialist with capabilities, tools, and workflow guidelines |
.github/ISSUE_TEMPLATE/agents/php-devloper.agent.md |
New agent template for PHP Developer focusing on PSR-12 standards and modern PHP practices |
.github/ISSUE_TEMPLATE/agents/mysql-mariadb.agent.md |
New agent template for MySQL/MariaDB Database Administrator with best practices and Cacti DB function references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $detail_messages = syslog_db_fetch_assoc("SELECT syslog.*, syslog_programs.program | ||
| FROM `" . $syslogdb_default . "`.`" . (($sm['mtype'] == 'main') ? 'syslog' : 'syslog_removed') . "` AS syslog | ||
| LEFT JOIN `" . $syslogdb_default . "`.`syslog_programs` | ||
| ON syslog.program_id=syslog_programs.program_id | ||
| WHERE syslog.seq IN (" . implode(',', array_map('intval', $seq_array)) . ") | ||
| ORDER BY syslog.logtime DESC"); |
There was a problem hiding this comment.
When grouping is enabled, this code executes an additional database query for every grouped message row that has multiple occurrences. This creates an N+1 query problem where N is the number of grouped message rows with multiple occurrences. For pages with many grouped messages, this could result in hundreds of additional database queries, significantly impacting performance. Consider fetching all detail messages in a single query before the loop or implementing pagination/lazy loading for the detail rows.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| MIN(syslog.logtime) AS first_logtime, | ||
| MAX(syslog.logtime) AS logtime, | ||
| MIN(syslog.seq) AS seq, | ||
| GROUP_CONCAT(syslog.seq ORDER BY syslog.logtime DESC SEPARATOR ',') AS seq_list |
There was a problem hiding this comment.
The GROUP_CONCAT function has a default maximum length limit (typically 1024 bytes in MySQL/MariaDB). If a message group contains a very large number of occurrences, the seq_list may be truncated silently, causing some detail messages to not be displayed when the group is expanded. Consider setting group_concat_max_len to a higher value before the query or implementing pagination for detail rows to handle large groups more gracefully.
There was a problem hiding this comment.
This is true. The GROUP_CONCAT function needs a session tuning to increase the length.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| --- | ||
| description: "This custom agent acts as a PHP developer, assisting with PHP code development, debugging, and optimization." | ||
| name: "PHP Developer" | ||
| tools: ["search/codebase", "edit/editFiles", "web/githubRepo", "vscode/extensions", "execute/getTerminalOutput", "web"] | ||
| model: "Claude Sonnet 4.5" | ||
| --- | ||
|
|
||
| # PHP Developer | ||
| You are a PHP Developer agent. Your role is to assist with PHP code development, debugging, and optimization. You have access to various tools to help you perform your tasks effectively. | ||
| You are to focus on PHP PSR-12 coding standards and best practices supporting modern PHP versions (PHP 8.1 and above). | ||
| Your other roles include: | ||
| - **Code Review:** Analyze PHP code for adherence to coding standards, best practices, and design patterns. | ||
| - **Debugging:** Identify and resolve bugs or issues in PHP code. | ||
| - **Performance Optimization:** Suggest and implement optimizations to improve the performance of PHP applications. | ||
| - **Testing:** Ensure that PHP code is well-tested, with appropriate unit tests and integration tests. | ||
| - **Documentation:** Verify that PHP code is well-documented, with clear comments and comprehensive documentation. | ||
| - **Security Best Practices:** Ensure that PHP code follows security best practices to prevent vulnerabilities. | ||
|
|
||
| ## Tools | ||
| You have access to the following tools to assist you in your tasks: | ||
| - **search/codebase:** Search through the codebase for relevant information or code snippets. | ||
| - **edit/editFiles:** Edit PHP code files to implement improvements or fixes. | ||
| - **githubRepo:** Interact with the GitHub repository to manage issues, pull requests, and code reviews. | ||
| - **extensions:** Utilize extensions that can enhance your capabilities in PHP development. | ||
| - **web:** Access the web for additional resources, documentation, or best practices. | ||
|
|
||
|
|
||
|
|
||
| ## The project in this repo calls on functions from the cacti project. You can find the cacti documentation and main github repo here: | ||
| - [Cacti GitHub Repository](https://github.com/Cacti/cacti/tree/1.2.x) | ||
| - [Cacti Documentation](https://www.github.com/Cacti/documentation) | ||
|
|
||
|
|
||
|
|
||
| ## Instructions | ||
| When assisting with tasks, follow these guidelines: | ||
| 1. **Understand the Request:** Clearly understand the user's request or issue before proceeding. | ||
| 2. **Gather Information:** Use the available tools to gather necessary information about the PHP codebase, coding standards, and existing issues. | ||
| 3. **Provide Solutions:** Offer clear and actionable solutions or recommendations based on best practices and your expertise. | ||
| 4. **Communicate Clearly:** Ensure that your explanations are clear and easy to understand, especially for users who may not be PHP experts. | ||
| 5. **Follow Up:** If necessary, follow up on previous tasks to ensure that PHP code issues have been resolved or improvements have been successfully implemented. |
There was a problem hiding this comment.
The filename contains a spelling error: "php-devloper.agent.md" should be "php-developer.agent.md" (missing 'e' in 'developer').
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| * Clear filter for alert rules view | ||
| */ | ||
| function clearFilterAlerts() { | ||
| strURL = 'syslog_alerts.php?clear=1&header=false'; |
There was a problem hiding this comment.
Variable strURL is used like a local variable, but is missing a declaration.
| * Import alert rule | ||
| */ | ||
| function importAlert() { | ||
| strURL = 'syslog_alerts.php?action=import&header=false'; |
There was a problem hiding this comment.
Variable strURL is used like a local variable, but is missing a declaration.
| * Apply filter for report rules view | ||
| */ | ||
| function applyFilterReports() { | ||
| strURL = 'syslog_reports.php?filter='+$('#filter').val()+'&enabled='+$('#enabled').val()+'&rows='+$('#rows').val()+'&page='+$('#page').val()+'&header=false'; |
There was a problem hiding this comment.
Variable strURL is used like a local variable, but is missing a declaration.
| * Clear filter for report rules view | ||
| */ | ||
| function clearFilterReports() { | ||
| strURL = 'syslog_reports.php?clear=1&header=false'; |
There was a problem hiding this comment.
Variable strURL is used like a local variable, but is missing a declaration.
| * Import report rule | ||
| */ | ||
| function importReport() { | ||
| strURL = 'syslog_reports.php?action=import&header=false'; |
There was a problem hiding this comment.
Variable strURL is used like a local variable, but is missing a declaration.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| MIN(syslog.logtime) AS first_logtime, | ||
| MAX(syslog.logtime) AS logtime, | ||
| MIN(syslog.seq) AS seq, | ||
| GROUP_CONCAT(syslog.seq ORDER BY syslog.logtime DESC SEPARATOR ',') AS seq_list |
There was a problem hiding this comment.
This is true. The GROUP_CONCAT function needs a session tuning to increase the length.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| **Documentation & Resources** | ||
| - [Cacti main repo](https://github.com/Cacti/cacti/tree/1.2.x) | ||
| - [cacti documentation](https://www.github.com/Cacti/documentation) |
There was a problem hiding this comment.
The URL 'www.github.com/Cacti/documentation' is inconsistent with standard GitHub URL formats. GitHub URLs should not include 'www.' and typically follow the pattern 'github.com/username/repo'. The correct URL should be either 'https://github.com/Cacti/documentation' or just 'github.com/Cacti/documentation' without the 'www.' prefix.
| - [cacti documentation](https://www.github.com/Cacti/documentation) | |
| - [cacti documentation](https://github.com/Cacti/documentation) |
| }); | ||
| }); | ||
|
|
||
| initSyslogRemoval(<?php print syslog_allow_edits() ? 'true':'false';?>); |
There was a problem hiding this comment.
The initSyslogRemoval function is being called with an allowEdits parameter on line 561, but this same function is also being called on line 504 in the edit action context. However, on line 504, the same allowEdits value is being passed, which means both the filter view and the edit view are sharing the same initialization. This might be incorrect - the filter view (line 561) should likely not receive the allowEdits parameter or should handle it differently, as the allowEdits logic in initSyslogRemoval() is specifically for disabling form elements in the edit context, not for the filter form.
| initSyslogRemoval(<?php print syslog_allow_edits() ? 'true':'false';?>); | |
| initSyslogRemoval(true); |
| */ | ||
| function changeTypes() { | ||
| if ($('#type').val() == 'sql') { | ||
| $('#message').prop('rows', 5); |
There was a problem hiding this comment.
The changeTypes() function sets the message textarea to 5 rows for SQL type, but this appears to be inconsistent. In syslog_alerts.php (line 664), the same function sets it to 6 rows for SQL type. This discrepancy should be verified - either both should use the same value, or if they genuinely need different values, the function should accept a parameter to handle page-specific requirements.
| */ | |
| function changeTypes() { | |
| if ($('#type').val() == 'sql') { | |
| $('#message').prop('rows', 5); | |
| * @param {number} [sqlRows=5] - Optional number of rows for SQL type (defaults to 5) | |
| */ | |
| function changeTypes(sqlRows) { | |
| var sqlRowsCount = (typeof sqlRows === 'number' && isFinite(sqlRows)) ? sqlRows : 5; | |
| if ($('#type').val() == 'sql') { | |
| $('#message').prop('rows', sqlRowsCount); |
| .syslog-group-toggle.fa-chevron-down { | ||
| transform: rotate(0deg); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Both CSS rules for .syslog-group-toggle.fa-chevron-up and .syslog-group-toggle.fa-chevron-down have identical transform properties (rotate(0deg)). This suggests no visual rotation difference between the two states. If the intention is to show a rotation animation or different visual states when toggling between chevron-up and chevron-down, one should likely have a different rotation value (e.g., rotate(180deg)). Otherwise, these redundant rules can be removed.
| .syslog-group-toggle.fa-chevron-down { | |
| transform: rotate(0deg); | |
| } |
| - **Critical:** ALWAYS use the `syslog_db_*` wrapper functions (defined in `database.php`) for all database operations. NEVER use standard Cacti `db_*` functions directly for syslog tables, as they will fail if a dedicated database is configured. | ||
| - **Integration:** The plugin integrates with Cacti via hooks defined in `setup.php`. | ||
| - **Poller Integration:** Background processes (`syslog_process.php`, `syslog_removal.php`) are triggered by Cacti's poller or run independently. | ||
| - **Syslog Reception:** Syslog messages are directly inserted into `syslog_incoming` table syslog_process.php then processes them. |
There was a problem hiding this comment.
The sentence structure is awkward. 'Syslog Reception: Syslog messages are directly inserted into syslog_incoming table syslog_process.php then processes them.' is missing punctuation and clarity. It should read something like: 'Syslog Reception: Syslog messages are directly inserted into the syslog_incoming table. The syslog_process.php script then processes them.' The period is missing between 'table' and 'syslog_process.php', and there should be an article ('The') before syslog_process.php for better readability.
| - **Syslog Reception:** Syslog messages are directly inserted into `syslog_incoming` table syslog_process.php then processes them. | |
| - **Syslog Reception:** Syslog messages are directly inserted into the `syslog_incoming` table. The `syslog_process.php` script then processes them. |
| @@ -0,0 +1,41 @@ | |||
| --- | |||
There was a problem hiding this comment.
The filename 'php-devloper.agent.md' contains a spelling error. 'devloper' should be spelled 'developer'. This affects the file name itself and should be corrected to maintain professionalism and consistency.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| $('#term').focus(); | ||
| }, | ||
| click: function(event, ui) { | ||
| checked=$(this).multiselect('widget').find('input:checked').length; |
There was a problem hiding this comment.
Variable checked is used like a local variable, but is missing a declaration.
| checked=$(this).multiselect('widget').find('input:checked').length; | |
| var checked = $(this).multiselect('widget').find('input:checked').length; |
| print "<td class='left' style='padding-left:30px;'>" . $dm['logtime'] . "</td>"; | ||
| print "<td class='left'>" . (isset($hosts[$dm['host_id']]) ? $hosts[$dm['host_id']] : __('Unknown', 'syslog')) . "</td>"; | ||
| print "<td class='left'>" . $dm['program'] . "</td>"; | ||
| print "<td class='left syslogMessage'>" . filter_value(title_trim($dm[$syslog_incoming_config['textField']], get_request_var_request('trimval')), get_request_var('rfilter')) . "</td>"; | ||
| print "<td class='left'>" . (isset($facilities[$dm['facility_id']]) ? $facilities[$dm['facility_id']] : __('Unknown', 'syslog')) . "</td>"; | ||
| print "<td class='left'>" . (isset($priorities[$dm['priority_id']]) ? $priorities[$dm['priority_id']] : __('Unknown', 'syslog')) . "</td>"; |
There was a problem hiding this comment.
The new grouped syslog detail rows print several fields directly into HTML without escaping (e.g., logtime, hosts[$dm['host_id']], and especially dm['program']), allowing untrusted syslog data such as the program/tag value to inject arbitrary HTML/JavaScript into the UI. An attacker who can send crafted syslog messages can persistently store a malicious program name that will execute in the browser of any user expanding a grouped row, leading to a stored XSS against authenticated users. To prevent this, ensure all dynamic values rendered in these <td> cells are passed through the same HTML-escaping helpers used elsewhere (e.g., html_escape/form_selectable_cell) before output.
| print "<td class='left' style='padding-left:30px;'>" . $dm['logtime'] . "</td>"; | |
| print "<td class='left'>" . (isset($hosts[$dm['host_id']]) ? $hosts[$dm['host_id']] : __('Unknown', 'syslog')) . "</td>"; | |
| print "<td class='left'>" . $dm['program'] . "</td>"; | |
| print "<td class='left syslogMessage'>" . filter_value(title_trim($dm[$syslog_incoming_config['textField']], get_request_var_request('trimval')), get_request_var('rfilter')) . "</td>"; | |
| print "<td class='left'>" . (isset($facilities[$dm['facility_id']]) ? $facilities[$dm['facility_id']] : __('Unknown', 'syslog')) . "</td>"; | |
| print "<td class='left'>" . (isset($priorities[$dm['priority_id']]) ? $priorities[$dm['priority_id']] : __('Unknown', 'syslog')) . "</td>"; | |
| print "<td class='left' style='padding-left:30px;'>" . html_escape($dm['logtime']) . "</td>"; | |
| print "<td class='left'>" . html_escape(isset($hosts[$dm['host_id']]) ? $hosts[$dm['host_id']] : __('Unknown', 'syslog')) . "</td>"; | |
| print "<td class='left'>" . html_escape($dm['program']) . "</td>"; | |
| print "<td class='left syslogMessage'>" . filter_value(title_trim($dm[$syslog_incoming_config['textField']], get_request_var_request('trimval')), get_request_var('rfilter')) . "</td>"; | |
| print "<td class='left'>" . html_escape(isset($facilities[$dm['facility_id']]) ? $facilities[$dm['facility_id']] : __('Unknown', 'syslog')) . "</td>"; | |
| print "<td class='left'>" . html_escape(isset($priorities[$dm['priority_id']]) ? $priorities[$dm['priority_id']] : __('Unknown', 'syslog')) . "</td>"; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
This pull request introduces new GitHub agent templates for code quality, PHP development, and MySQL/MariaDB administration, and refactors the syslog plugin's frontend code to improve maintainability and consistency. The syslog plugin changes include centralizing JavaScript includes, refactoring inline JavaScript to use new initialization functions, and adding new CSS classes for better UI/UX.
Agent Templates:
.github/ISSUE_TEMPLATE/agents/code-quality.agent.mdfor a Code Quality Specialist agent, detailing capabilities, tools, and workflow guidelines..github/ISSUE_TEMPLATE/agents/php-devloper.agent.mdfor a PHP Developer agent, focusing on PSR-12 standards, modern PHP, and Cacti integration..github/ISSUE_TEMPLATE/agents/mysql-mariadb.agent.mdfor a MySQL/MariaDB Database Administrator agent, including best practices, Cacti DB functions, and documentation links.Syslog Plugin Frontend Refactor:
syslog_include_js()function infunctions.php, and updatedsyslog_alerts.php,syslog_reports.php, andsyslog_removal.phpto use this function for consistent JS loading. [1] [2] [3] [4] [5] [6] [7]syslog_alerts.php,syslog_reports.php, andsyslog_removal.phpto replace inline functions with calls to new initialization functions (initSyslogAlerts,initSyslogReports,initSyslogRemoval), improving maintainability and separation of concerns. [1] [2] [3] [4]syslog-detail-rowclass for table rows and new CSS for row hover highlighting and group toggles insyslog.css, improving user experience for grouped syslog messages. [1] [2]Documentation Improvements:
copilot-instructions.mdto clarify syslog message handling and add links to Cacti documentation and main repository. [1] [2]