Next release: feat: Implement DevicesHistory feature with triggers and history tracking#1701
Open
jokob-sk wants to merge 4 commits into
Open
Next release: feat: Implement DevicesHistory feature with triggers and history tracking#1701jokob-sk wants to merge 4 commits into
jokob-sk wants to merge 4 commits into
Conversation
…king - Added db_history.py to manage DevicesHistory table and triggers for INSERT and UPDATE operations. - Created device_history_instance.py for querying and grouping DevicesHistory records. - Developed change_history.php for displaying device change history with filtering and pagination. - Introduced skel_device_details_tab_history.php for skeleton loading state in device details tab. - Added unit tests in test_device_history.py to validate trigger functionality and history management. - Implemented filter population and pagination in the change history UI.
Contributor
📝 WalkthroughWalkthroughAdds device-change auditing, grouped history queries, frontend history views, retention settings, cleanup/pruning, localization, docs, tests, and related skill-guide documentation. ChangesDevice Change History Feature
AI Skill Guide Documentation Updates
Sequence Diagram(s)sequenceDiagram
participant Devices as Devices table
participant Trigger as SQLite trigger
participant DevicesHistory as DevicesHistory
participant Frontend as changeLogCore.php
participant API as GraphQL / REST APIs
participant Model as DevicesHistoryInstance
Devices->>Trigger: INSERT or UPDATE row
Trigger->>DevicesHistory: write per-field change row
Frontend->>API: request grouped history and filter values
API->>Model: query grouped rows / distinct filters
Model->>DevicesHistory: SELECT history rows
DevicesHistory-->>Model: matching rows
Model-->>API: grouped events / filter values
API-->>Frontend: history data for DataTable
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
jokob-sk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
docs/SETTINGS_SYSTEM.md (1)
58-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShow both localization hooks in the example.
The example only demonstrates the
namelookup, but the new settings strings already exist as both<KEY>_nameand<KEY>_description. Including both references here will keep new plugin settings from being half-localized.♻️ Proposed doc fix
"name": [ { "string": "_GLOBAL_LANG_FILES_" } -] +], +"description": [ + { + "string": "_GLOBAL_LANG_FILES_" + } +]🤖 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 `@docs/SETTINGS_SYSTEM.md` around lines 58 - 66, The SETTINGS_SYSTEM example only shows the `name` localization hook and omits the matching `description` hook, so update the example to demonstrate both references together. Use the existing localization pattern in the doc around the `config.json` example to show how both `<KEY>_name` and `<KEY>_description` should be declared, so readers can localize plugin settings fully..gemini/skills/testing-workflow/SKILL.md (2)
29-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse repo-relative commands in the examples.
Every command example here hard-codes
/workspaces/NetAlertX(and/workspaces/NetAlertX/.devcontainer/...). That contradicts the repo-root convention for Gemini skills and makes the examples brittle. Please switch these to relative paths or explicitly scope them to the devcontainer. Based on learnings: command examples in.gemini/skillsshould omit the absolute/workspaces/NetAlertXprefix.🤖 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 @.gemini/skills/testing-workflow/SKILL.md around lines 29 - 85, The command examples in the testing workflow skill hard-code the repository root, which conflicts with the repo-relative convention. Update the examples in SKILL.md to use relative paths for pytest and setup commands, and ensure any references tied to the testing flow use the same repo-root style consistently. Locate the affected examples around the testing sections and the Authentication & Environment Reset steps.Source: Learnings
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the Copilot-only skill handoff.
.gemini/skillsdocs should not instruct readers to callactivate_skill("devcontainer-management"); that's a Copilot/devcontainer workflow, not a Gemini CLI step. Please restate this as plain devcontainer guidance or move it to the Copilot skill. Based on learnings: Gemini-side skill docs should only reference Gemini-available tools.🤖 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 @.gemini/skills/testing-workflow/SKILL.md around lines 19 - 23, The testing-workflow skill currently contains a Copilot-specific handoff that tells readers to call activate_skill("devcontainer-management"), which is not valid Gemini CLI guidance. Update the relevant guidance in SKILL.md to describe the devcontainer step in plain language using Gemini-available tooling, or remove it from this skill and relocate it to the Copilot-side skill; keep the surrounding host-machine/container detection logic consistent with the existing testing-workflow instructions.Source: Learnings
.gemini/skills/skills-index/SKILL.md (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid naming
testFailurein the Gemini-side index.This row pulls a devcontainer-only tool into a
.gemini/skillsdocument. If the goal is just to point to the richer Copilot counterpart, keep the description environment-neutral so the Gemini index doesn't advertise tools it can't use. Based on learnings: Gemini-side skill docs should avoid devcontainer-only tool references.🤖 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 @.gemini/skills/skills-index/SKILL.md at line 21, The `testing-workflow` row in `SKILL.md` should not mention the devcontainer-only `testFailure` tool or other Copilot-specific tooling. Update the `Testing` entry to keep the Gemini-side index environment-neutral while still pointing to the richer counterpart, so the `testing-workflow` description only references capabilities available in Gemini.Source: Learnings
server/db/db_history.py (2)
28-74: 📐 Maintainability & Code Quality | 🔵 TrivialSingle source of truth for tracked-field list would prevent drift.
_HIST_FIELDShere,_DEV_HIST_TRACKED_DEFAULTinserver/initialise.py, and theDEV_HIST_TRACKEDdefault inback/app.confmust all be kept in sync by hand. They currently match (41 fields each), but any future addition/removal in one place without the others will silently desync "trackable" fields from "actually audited" fields.Consider deriving
_HIST_FIELDS's field-name list (or the settings default) from a single shared constant module both files import.🤖 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 `@server/db/db_history.py` around lines 28 - 74, The tracked-field definitions are duplicated across _HIST_FIELDS in db_history, _DEV_HIST_TRACKED in initialise, and the DEV_HIST_TRACKED default, so they can drift apart. Introduce a single shared source for the field-name list (for example, a constant module imported by db_history and initialise) and have the settings default derive from that shared list instead of hardcoding it in multiple places. Keep the existing symbols _HIST_FIELDS and _DEV_HIST_TRACKED aligned through that shared constant.
85-88: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPer-field Settings subquery repeated up to 41× per Devices write.
Each of the 41 per-field
INSERT..SELECTblocks re-runs the_TRACKED_CHECKsubquery againstSettingsindependently, in addition to the single_HIST_DAYS_GUARDsubquery in the trigger'sWHENclause. On every Devices row UPDATE/INSERT (which happens on every scan cycle for active devices) this issues up to 41 near-duplicate lookups against theSettingstable.💡 Possible direction
Consider normalizing
DEV_HIST_TRACKEDinto a dedicated lookup structure (e.g., a smallDevHistTrackedFieldstable populated on settings import) and joining/EXISTS against it once per trigger firing, rather than doing 41 independentinstr()string searches against a serialized settings value on every row write.Also applies to: 91-143
🤖 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 `@server/db/db_history.py` around lines 85 - 88, The Devices history trigger is repeatedly re-running the same DEV_HIST_TRACKED lookup for each per-field block, causing many duplicate Settings subqueries on every write. Update the db_history trigger generation around _TRACKED_CHECK and the per-field INSERT..SELECT blocks to resolve tracked fields once per trigger firing, ideally by using a dedicated lookup structure such as DevHistTrackedFields or an equivalent precomputed EXISTS/join check, and then reuse that result across all field writes.server/database.py (1)
242-247: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffFragile trigger-recreation coupling with
AppEvent_obj.drop_all_triggers().The re-creation after
AppEvent_obj(self)correctly compensates for the trigger wipe, but it relies on every future trigger-creating module remembering to hook in similarly afterAppEvent_obj. Worth considering, longer-term, havingdrop_all_triggers()scoped to only the triggers it actually owns (or an explicit trigger registry) instead of an indiscriminate drop-all.Not blocking — the current fix is correct and well-commented.
🤖 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 `@server/database.py` around lines 242 - 247, The trigger recreation in database initialization is currently compensating for AppEvent_obj.drop_all_triggers() wiping unrelated triggers, which creates fragile coupling. Update the trigger cleanup flow so AppEvent_obj.drop_all_triggers() only removes triggers it owns, or introduce an explicit trigger registry/ownership check, and keep ensure_deviceshistory_triggers(self.sql) as the device-history re-create path tied to the database bootstrap sequence.server/api_server/graphql_endpoint.py (1)
109-109: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlind
except Exceptionswallows real errors with minimal diagnostics.Ruff flags both catches (BLE001). Returning an empty
DeviceHistoryResulton any failure (including bugs/DB errors) makes the UI look like "no history" instead of surfacing a real failure, and only{e}is logged (no traceback), making root-causing production issues harder.Also applies to: 131-131
🤖 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 `@server/api_server/graphql_endpoint.py` at line 109, The broad exception handlers in the GraphQL endpoint are hiding real failures and only logging a minimal message. Replace the two `except Exception` blocks in the device history resolver path with narrower error handling or re-raise unexpected errors, and keep returning `DeviceHistoryResult` only for the intended “no data” cases. In the logging path, include full exception context/traceback instead of just `{e}` so failures in the `graphql_endpoint` flow are diagnosable.Source: Linters/SAST tools
🤖 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 @.gemini/skills/initiative-start/reasearch-skill.md:
- Around line 24-26: The skill reference examples in this document use an
incorrect repo-root path prefix, so update the `.gemini/skills` guidance to
reference `.github/...` without the leading slash. Fix the path strings in the
relevant list items in `reasearch-skill.md`, keeping the examples consistent
with the rest of the skill docs and the repository-root-relative convention.
In `@front/change_history.php`:
- Line 42: The change history table header for Device is hardcoded instead of
using localization like the other headers. Update the table header in the change
history view to use the existing lang(...) translation pattern, matching the
surrounding header labels in the same table.
- Around line 109-123: Both AJAX requests in the change history flow are missing
failure handling, so the loading state can persist indefinitely; update the
request logic in the change history code path to add an error callback for each
$.ajax call, and make sure it clears the spinner/loading row in
`#changeHistoryBody` and shows a user-facing failure state when the request in
change_history.php fails. Keep the fix in the same AJAX handlers that currently
call renderTable and updatePagination so both the initial load and the later
request paths handle auth, network, and server errors consistently.
- Around line 25-32: The change history filters in the two select elements
currently call loadChangeHistory() without resetting pagination, which can leave
the table empty when a filter shrinks the result set. Update the filter change
handling in change_history.php so that the pagination state variable _chPage is
reset to the first page before loadChangeHistory() runs, and make sure both
filter controls use the same reset behavior.
- Line 110: The change history page is hardcoding backend URLs instead of
honoring the configured API base. Update the URL construction in
change_history.php so both the GraphQL endpoint and the history filters endpoint
are built from getApiBase(), matching the other history pages and avoiding
direct use of /server/graphql and /server/devices/history/filters.
- Around line 133-148: The device link in the change history table is still
pointing to an empty mac query, so it cannot open the correct device details.
Update the anchor generation in change_history.php where the row HTML is built
to pass the actual device identifier expected by deviceDetails.php, using the
same value already available as g.devGUID (or the proper MAC field if present),
and remove the unused data-guid attribute unless it is needed elsewhere.
In `@front/deviceDetails.php`:
- Around line 687-707: The AJAX request in the history loading flow is missing
an error handler, so failures leave the inline spinner and history skeleton
stuck. Update the $.ajax call in the device history logic to include an error
callback alongside success, and use it to clear the loading state for
historyTableBody and hide `#skel-tab-history` on auth/network/5xx failures. Keep
the fix localized to the history fetch routine that calls _renderHistoryTable,
_updateHistoryPagination, and _populateHistoryFilters so all failure paths clean
up the UI.
- Around line 174-181: The history filter dropdowns in the device details view
trigger loadHistoryData() without resetting _histPage, so changing a filter can
keep the user on an out-of-range page. Update the onchange behavior for
histFilterBy and histFilterCol so the pagination state is reset before reloading
history, matching the fix used in change_history.php and ensuring
loadHistoryData() starts from the first page after any filter change.
In `@front/plugins/db_cleanup/script.py`:
- Around line 163-169: Wrap the DevicesHistory cleanup in `cleanup_database()`
so a failure in `DevicesHistoryInstance().prune_history(DEV_HIST_DAYS)` does not
abort the rest of the database cleanup. Add a local try/except around the
existing `DEV_HIST_DAYS` block, log the exception with `mylog`, and then
continue to the remaining prune, dedupe, checkpoint, `ANALYZE`, and `VACUUM`
steps. Keep the behavior of the `DevicesHistoryInstance` path the same on
success, but make sure errors are isolated to this step only.
- Line 38: The DEV_HIST_DAYS setting in the db cleanup script is using a truthy
fallback that overrides an explicit 0 value. Update the assignment in the
module-level configuration near get_setting_value("DEV_HIST_DAYS") so it only
falls back to 14 when the setting is None, preserving 0 as a valid disable value
for prune_history().
In `@server/api_server/api_server_start.py`:
- Around line 776-789: The api_devices_history_filters REST handler currently
calls DevicesHistoryInstance().get_available_filter_values() without any
exception handling, so DB failures can bubble up as an unhandled Flask 500 and
break the JSON contract. Wrap the logic in api_devices_history_filters with
try/except similar to the GraphQL history resolvers, and on error return a JSON
response with success false and an empty or safe fallback data payload so the
frontend can keep using resp.data consistently.
In `@server/api_server/graphql_endpoint.py`:
- Around line 67-134: The history resolvers are passing the client-provided
limit through without an upper bound, unlike the pagination pattern used
elsewhere in this file. Update resolve_allDeviceHistoryGrouped (and match
resolve_deviceHistoryGrouped if needed) to clamp limit with the existing
_MAX_LIMIT before calling DevicesHistoryInstance methods, so large requests
cannot trigger oversized grouped-history payloads. Keep the change localized to
the resolver methods and preserve the current default limits.
In `@server/models/device_history_instance.py`:
- Around line 174-216: The grouped history pagination in _query_grouped
currently loads and groups every matching row before slicing, so move pagination
into the database instead of applying offset/limit only to grouped_list. Update
_query_grouped to first select the page of distinct group keys (timestamp,
changedBy, devGUID) with ORDER BY plus LIMIT/OFFSET, then fetch only the rows
for those keys to assemble each group’s changes. Keep the existing filter
behavior from _build_clauses and ensure any changedColumn filtering still
matches the intended group-selection semantics.
---
Nitpick comments:
In @.gemini/skills/skills-index/SKILL.md:
- Line 21: The `testing-workflow` row in `SKILL.md` should not mention the
devcontainer-only `testFailure` tool or other Copilot-specific tooling. Update
the `Testing` entry to keep the Gemini-side index environment-neutral while
still pointing to the richer counterpart, so the `testing-workflow` description
only references capabilities available in Gemini.
In @.gemini/skills/testing-workflow/SKILL.md:
- Around line 29-85: The command examples in the testing workflow skill
hard-code the repository root, which conflicts with the repo-relative
convention. Update the examples in SKILL.md to use relative paths for pytest and
setup commands, and ensure any references tied to the testing flow use the same
repo-root style consistently. Locate the affected examples around the testing
sections and the Authentication & Environment Reset steps.
- Around line 19-23: The testing-workflow skill currently contains a
Copilot-specific handoff that tells readers to call
activate_skill("devcontainer-management"), which is not valid Gemini CLI
guidance. Update the relevant guidance in SKILL.md to describe the devcontainer
step in plain language using Gemini-available tooling, or remove it from this
skill and relocate it to the Copilot-side skill; keep the surrounding
host-machine/container detection logic consistent with the existing
testing-workflow instructions.
In `@docs/SETTINGS_SYSTEM.md`:
- Around line 58-66: The SETTINGS_SYSTEM example only shows the `name`
localization hook and omits the matching `description` hook, so update the
example to demonstrate both references together. Use the existing localization
pattern in the doc around the `config.json` example to show how both
`<KEY>_name` and `<KEY>_description` should be declared, so readers can localize
plugin settings fully.
In `@server/api_server/graphql_endpoint.py`:
- Line 109: The broad exception handlers in the GraphQL endpoint are hiding real
failures and only logging a minimal message. Replace the two `except Exception`
blocks in the device history resolver path with narrower error handling or
re-raise unexpected errors, and keep returning `DeviceHistoryResult` only for
the intended “no data” cases. In the logging path, include full exception
context/traceback instead of just `{e}` so failures in the `graphql_endpoint`
flow are diagnosable.
In `@server/database.py`:
- Around line 242-247: The trigger recreation in database initialization is
currently compensating for AppEvent_obj.drop_all_triggers() wiping unrelated
triggers, which creates fragile coupling. Update the trigger cleanup flow so
AppEvent_obj.drop_all_triggers() only removes triggers it owns, or introduce an
explicit trigger registry/ownership check, and keep
ensure_deviceshistory_triggers(self.sql) as the device-history re-create path
tied to the database bootstrap sequence.
In `@server/db/db_history.py`:
- Around line 28-74: The tracked-field definitions are duplicated across
_HIST_FIELDS in db_history, _DEV_HIST_TRACKED in initialise, and the
DEV_HIST_TRACKED default, so they can drift apart. Introduce a single shared
source for the field-name list (for example, a constant module imported by
db_history and initialise) and have the settings default derive from that shared
list instead of hardcoding it in multiple places. Keep the existing symbols
_HIST_FIELDS and _DEV_HIST_TRACKED aligned through that shared constant.
- Around line 85-88: The Devices history trigger is repeatedly re-running the
same DEV_HIST_TRACKED lookup for each per-field block, causing many duplicate
Settings subqueries on every write. Update the db_history trigger generation
around _TRACKED_CHECK and the per-field INSERT..SELECT blocks to resolve tracked
fields once per trigger firing, ideally by using a dedicated lookup structure
such as DevHistTrackedFields or an equivalent precomputed EXISTS/join check, and
then reuse that result across all field writes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 131077e3-689c-435f-a899-245ba5aca21b
📒 Files selected for processing (30)
.gemini/skills/initiative-start/reasearch-skill.md.gemini/skills/plugin-development/plugin-skill.md.gemini/skills/project-navigation/SKILL.md.gemini/skills/settings/SKILL.md.gemini/skills/skills-index/SKILL.md.gemini/skills/testing-workflow/SKILL.md.github/copilot-instructions.md.github/skills/database-patterns/SKILL.md.github/skills/mcp-activation/SKILL.md.github/skills/settings-management/SKILL.md.github/skills/skills-overview/SKILL.mdback/app.confdocs/API_GRAPHQL.mddocs/SETTINGS_SYSTEM.mdfront/change_history.phpfront/deviceDetails.phpfront/php/templates/header.phpfront/php/templates/language/en_us.jsonfront/php/templates/skel_device_details_tab_history.phpfront/plugins/db_cleanup/script.pyserver/api_server/api_server_start.pyserver/api_server/graphql_endpoint.pyserver/api_server/graphql_types.pyserver/database.pyserver/db/db_history.pyserver/initialise.pyserver/models/device_history_instance.pyserver/models/device_instance.pytest/backend/test_device_history.pytest/db_test_helpers.py
| HRS_TO_KEEP_OFFDEV = int(get_setting_value("HRS_TO_KEEP_OFFDEV")) | ||
| DAYS_TO_KEEP_EVENTS = int(get_setting_value("DAYS_TO_KEEP_EVENTS")) | ||
| CLEAR_NEW_FLAG = get_setting_value("CLEAR_NEW_FLAG") | ||
| DEV_HIST_DAYS = int(get_setting_value("DEV_HIST_DAYS") or 14) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## script.py relevant slice\n'
cat -n front/plugins/db_cleanup/script.py | sed -n '1,140p'
printf '\n## device_history_instance.py relevant slice\n'
cat -n server/models/device_history_instance.py | sed -n '1,220p'
printf '\n## search for get_setting_value and DEV_HIST_DAYS/CLEAR_NEW_FLAG usages\n'
rg -n "get_setting_value|DEV_HIST_DAYS|CLEAR_NEW_FLAG" front/plugins/db_cleanup script.py server/models -SRepository: netalertx/NetAlertX
Length of output: 19699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## locate helper.py\n'
fd -a '^helper\.py$|/helper\.py$|helper\.py$' .
printf '\n## inspect get_setting_value implementation\n'
FILE=$(fd -a '^helper\.py$|/helper\.py$|helper\.py$' . | head -n 1)
cat -n "$FILE" | sed -n '1,260p'
printf '\n## search for return types / default handling in helper.py\n'
rg -n "def get_setting_value|return .*None|int\(|float\(|strip\(|lower\(" "$FILE" -n -SRepository: netalertx/NetAlertX
Length of output: 11292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=server/helper.py
printf '\n## get_setting_value definition\n'
rg -n "def get_setting_value|def set_setting_value|class .*Settings" "$FILE" -n -S
printf '\n## helper.py relevant slice\n'
cat -n "$FILE" | sed -n '1,260p'Repository: netalertx/NetAlertX
Length of output: 11790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=server/helper.py
printf '\n## setting_value_to_python_type definition\n'
rg -n "def setting_value_to_python_type" "$FILE" -n -S
LINE=$(rg -n "def setting_value_to_python_type" "$FILE" -n -S | cut -d: -f1 | head -n 1)
START=$((LINE-20))
END=$((LINE+120))
sed -n "${START},${END}p" "$FILE" | cat -nRepository: netalertx/NetAlertX
Length of output: 6474
Preserve DEV_HIST_DAYS=0 here. get_setting_value("DEV_HIST_DAYS") can return a native 0, and prune_history() treats <= 0 as disabled; or 14 turns that explicit disable back into 14. Use an is not None fallback instead. front/plugins/db_cleanup/script.py:38
🤖 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 `@front/plugins/db_cleanup/script.py` at line 38, The DEV_HIST_DAYS setting in
the db cleanup script is using a truthy fallback that overrides an explicit 0
value. Update the assignment in the module-level configuration near
get_setting_value("DEV_HIST_DAYS") so it only falls back to 14 when the setting
is None, preserving 0 as a valid disable value for prune_history().
Comment on lines
+163
to
+169
| # ----------------------------------------------------- | ||
| # Cleanup DevicesHistory | ||
| if DEV_HIST_DAYS > 0: | ||
| mylog("verbose", f"[{pluginName}] DevicesHistory: Delete rows older than {DEV_HIST_DAYS} days") | ||
| deleted = DevicesHistoryInstance().prune_history(DEV_HIST_DAYS) | ||
| mylog("verbose", [f"[{pluginName}] DevicesHistory deleted rows: {deleted}"]) | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the cleanup section around the reported lines.
ast-grep outline front/plugins/db_cleanup/script.py --view expanded || true
wc -l front/plugins/db_cleanup/script.py
sed -n '130,240p' front/plugins/db_cleanup/script.py
# Inspect the helper being called to understand whether it may raise and whether it opens its own connection.
fd -a 'DevicesHistoryInstance' front || true
rg -n "class DevicesHistoryInstance|def prune_history|prune_history\(" front -A 20 -B 10
# Look for surrounding try/except in the cleanup flow.
rg -n "try:|except Exception|REINDEX|ANALYZE|VACUUM|WAL checkpoint|Plugins_Objects|clear-new-flag|offline device|new/offline" front/plugins/db_cleanup/script.py -A 8 -B 8Repository: netalertx/NetAlertX
Length of output: 11586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the helper implementation and inspect its error handling / DB access.
rg -n "class DevicesHistoryInstance|def prune_history|prune_history\(" front -A 40 -B 20Repository: netalertx/NetAlertX
Length of output: 5658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the helper implementation and inspect the surrounding code.
fd -a 'DevicesHistory' front || true
rg -n "class DevicesHistoryInstance|def prune_history|DevicesHistoryInstance\(" front -A 40 -B 20
# If the helper lives in a different file, show all mentions so the call path is clear.
rg -n "prune_history|DevicesHistoryInstance|DevicesHistory" . -g '!**/.git/**' -A 8 -B 8Repository: netalertx/NetAlertX
Length of output: 47111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' front/plugins/db_cleanup/script.pyRepository: netalertx/NetAlertX
Length of output: 4983
Wrap prune_history() so a failure doesn’t stop the rest of cleanup.
cleanup_database() has no handler here, so any exception from DevicesHistoryInstance().prune_history(DEV_HIST_DAYS) will skip the remaining device pruning, dedupe, checkpoint, ANALYZE, and VACUUM steps.
🤖 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 `@front/plugins/db_cleanup/script.py` around lines 163 - 169, Wrap the
DevicesHistory cleanup in `cleanup_database()` so a failure in
`DevicesHistoryInstance().prune_history(DEV_HIST_DAYS)` does not abort the rest
of the database cleanup. Add a local try/except around the existing
`DEV_HIST_DAYS` block, log the exception with `mylog`, and then continue to the
remaining prune, dedupe, checkpoint, `ANALYZE`, and `VACUUM` steps. Keep the
behavior of the `DevicesHistoryInstance` path the same on success, but make sure
errors are isolated to this step only.
Comment on lines
+776
to
+789
| @app.route("/devices/history/filters", methods=["GET"]) | ||
| @validate_request( | ||
| operation_id="get_device_history_filters", | ||
| summary="Get Device History Filter Values", | ||
| description="Return distinct changedBy and changedColumn values available in DevicesHistory. Optionally scope to a single device with ?devGuid=<guid>.", | ||
| tags=["devices"], | ||
| auth_callable=is_authorized | ||
| ) | ||
| def api_devices_history_filters(payload=None): | ||
| dev_guid = request.args.get("devGuid") or None | ||
| filters = DevicesHistoryInstance().get_available_filter_values(devGUID=dev_guid) | ||
| return jsonify({"success": True, "data": filters}) | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No error handling around the DB call.
Unlike the GraphQL history resolvers (which wrap the same kind of DB access in try/except and return a safe fallback), this REST handler has no error handling. An exception here (DB error, etc.) will surface as an unhandled Flask 500, likely non-JSON, breaking the frontend's resp.data assumption.
🛡️ Proposed fix
def api_devices_history_filters(payload=None):
dev_guid = request.args.get("devGuid") or None
- filters = DevicesHistoryInstance().get_available_filter_values(devGUID=dev_guid)
- return jsonify({"success": True, "data": filters})
+ try:
+ filters = DevicesHistoryInstance().get_available_filter_values(devGUID=dev_guid)
+ return jsonify({"success": True, "data": filters})
+ except Exception as e:
+ mylog("none", f"[api] api_devices_history_filters error: {e}")
+ return jsonify({"success": False, "data": {"changedBy": [], "changedColumn": []}}), 500📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @app.route("/devices/history/filters", methods=["GET"]) | |
| @validate_request( | |
| operation_id="get_device_history_filters", | |
| summary="Get Device History Filter Values", | |
| description="Return distinct changedBy and changedColumn values available in DevicesHistory. Optionally scope to a single device with ?devGuid=<guid>.", | |
| tags=["devices"], | |
| auth_callable=is_authorized | |
| ) | |
| def api_devices_history_filters(payload=None): | |
| dev_guid = request.args.get("devGuid") or None | |
| filters = DevicesHistoryInstance().get_available_filter_values(devGUID=dev_guid) | |
| return jsonify({"success": True, "data": filters}) | |
| `@app.route`("/devices/history/filters", methods=["GET"]) | |
| `@validate_request`( | |
| operation_id="get_device_history_filters", | |
| summary="Get Device History Filter Values", | |
| description="Return distinct changedBy and changedColumn values available in DevicesHistory. Optionally scope to a single device with ?devGuid=<guid>.", | |
| tags=["devices"], | |
| auth_callable=is_authorized | |
| ) | |
| def api_devices_history_filters(payload=None): | |
| dev_guid = request.args.get("devGuid") or None | |
| try: | |
| filters = DevicesHistoryInstance().get_available_filter_values(devGUID=dev_guid) | |
| return jsonify({"success": True, "data": filters}) | |
| except Exception as e: | |
| mylog("none", f"[api] api_devices_history_filters error: {e}") | |
| return jsonify({"success": False, "data": {"changedBy": [], "changedColumn": []}}), 500 |
🤖 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 `@server/api_server/api_server_start.py` around lines 776 - 789, The
api_devices_history_filters REST handler currently calls
DevicesHistoryInstance().get_available_filter_values() without any exception
handling, so DB failures can bubble up as an unhandled Flask 500 and break the
JSON contract. Wrap the logic in api_devices_history_filters with try/except
similar to the GraphQL history resolvers, and on error return a JSON response
with success false and an empty or safe fallback data payload so the frontend
can keep using resp.data consistently.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/models/device_history_instance.py (2)
18-31: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConnections leak on query failure.
Neither
_fetchallnor_executeclose the connection ifexecute()raises (e.g., locked DB, malformed query). Under load/errors this can exhaust SQLite connections.🔧 Proposed fix
def _fetchall(self, query, params=()): conn = get_temp_db_connection() - rows = conn.execute(query, params).fetchall() - conn.close() - return [dict(r) for r in rows] + try: + rows = conn.execute(query, params).fetchall() + return [dict(r) for r in rows] + finally: + conn.close() def _execute(self, query, params=()): conn = get_temp_db_connection() - cur = conn.cursor() - cur.execute(query, params) - affected = cur.rowcount - conn.commit() - conn.close() - return affected + try: + cur = conn.cursor() + cur.execute(query, params) + affected = cur.rowcount + conn.commit() + return affected + finally: + conn.close()🤖 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 `@server/models/device_history_instance.py` around lines 18 - 31, Both `_fetchall` and `_execute` can leak the temp SQLite connection when `execute()` raises, so update these helpers to always close the connection even on failure. Use `get_temp_db_connection()` in `DeviceHistoryInstance`, and wrap the query/commit path in a try/finally so `conn.close()` runs regardless of success or exception; keep the existing return values from `_fetchall` and `_execute` unchanged.
118-136: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
get_total_group_countshould takesearchinto account.
get_grouped_history/get_all_grouped_historyalready filter bypaging["search"], butget_total_group_count()still omits it, socountcan exceed the number of groups actually shown. Threadsearchthrough the twoget_total_group_count(...)calls inserver/api_server/graphql_endpoint.pyand add it toserver/models/device_history_instance.py:get_total_group_count.🤖 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 `@server/models/device_history_instance.py` around lines 118 - 136, `get_total_group_count` is missing the `search` filter, so its count can differ from the grouped history results. Update `device_history_instance.get_total_group_count` to accept `search` and include it in the clause-building logic alongside `devGUID`, `changedColumn`, and `changedBy`, then thread the same `paging["search"]` argument through both calls in `graphql_endpoint.py` that invoke `get_total_group_count`. Use the existing `get_grouped_history` and `get_all_grouped_history` filtering pattern as the reference so the count matches the displayed groups.
♻️ Duplicate comments (2)
server/api_server/graphql_endpoint.py (1)
78-158: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMissing upper bound on
limitfor history queries.Same concern raised previously:
deviceHistoryGroupedpasses the client-suppliedlimitstraight through toDevicesHistoryInstancewithout capping it, unlikeapply_common_paginationelsewhere in this codebase which enforcesmin(options.limit, _MAX_LIMIT). Combined with_query_groupedfetching the full matching set before slicing, a largelimitcan force very large per-request payloads/memory use.⚡ Proposed fix
try: mylog("none", f"[HISTORY] unified resolver devGUID={devGUID} changedColumn={changedColumn} changedBy={changedBy}") h = DevicesHistoryInstance() paging = extract_paging(options) + paging["limit"] = min(paging["limit"], _MAX_LIMIT)Best applied inside
extract_pagingitself (see companion note in graphql_helpers.py) so all history callers get the cap consistently.🤖 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 `@server/api_server/graphql_endpoint.py` around lines 78 - 158, The unified history resolver is forwarding the client’s `limit` from `extract_paging` into `resolve_deviceHistoryGrouped` without any maximum cap, so large requests can still overwhelm `DevicesHistoryInstance`. Update `extract_paging` to enforce the same upper bound used by `apply_common_pagination` (cap `options.limit` with `_MAX_LIMIT`) so `resolve_deviceHistoryGrouped`, `get_grouped_history`, and `get_all_grouped_history` all receive bounded pagination values consistently.server/models/device_history_instance.py (1)
196-223: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftGrouped-history pagination still fetches the entire filtered result set before slicing.
This is the same concern raised on a prior commit:
_query_groupedissues no SQLLIMIT/OFFSET, fetching every row matching the filter before grouping, sorting, and only then slicinggrouped_list[start:end]. Cost is O(total matching rows) per page request, and the table grows monotonically since triggers insert a row per changed tracked field on every device update.💡 Possible direction (unchanged from previous review)
Push pagination into SQL by first selecting the page of distinct group keys (
timestamp, changedBy, devGUID) withORDER BY ... LIMIT ? OFFSET ?, then fetching only the detail rows for those keys — while preserving current filter semantics (e.g. whetherchangedColumnnarrows the "changes" list vs. only gates group inclusion).Also applies to: 279-285
🤖 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 `@server/models/device_history_instance.py` around lines 196 - 223, _query_grouped in device_history_instance.py still paginates after fetching all matching history rows, so move pagination into SQL instead of slicing grouped_list in Python. Update _query_grouped to first select the page of distinct group keys (timestamp, changedBy, devGUID) using the existing filters from _build_clauses plus ORDER BY and LIMIT/OFFSET, then fetch only the rows for those keys and build the grouped result. Preserve the current changedColumn/search semantics and keep the public query methods that rely on _query_grouped aligned with the new SQL-based pagination.
🧹 Nitpick comments (5)
server/api_server/graphql_helpers.py (1)
144-167: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider enforcing
_MAX_LIMIThere rather than in each caller.
extract_pagingis the shared entry point for history paging, and this file already defines_MAX_LIMIT/_DEFAULT_LIMITused byapply_common_pagination. Applying the cap here once would close the missing-limit-bound gap inresolve_deviceHistoryGrouped(see companion comment in graphql_endpoint.py) for all current and future callers.♻️ Proposed refactor
def extract_paging(options): if not options: return { "page": 1, - "limit": 50, + "limit": _DEFAULT_LIMIT, "offset": 0, "sort": [], "search": None } page = getattr(options, "page", 1) or 1 - limit = getattr(options, "limit", 50) or 50 + limit = min(getattr(options, "limit", _DEFAULT_LIMIT) or _DEFAULT_LIMIT, _MAX_LIMIT) search = getattr(options, "search", None) sort = getattr(options, "sort", []) or []🤖 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 `@server/api_server/graphql_helpers.py` around lines 144 - 167, `extract_paging` is the shared pagination entry point, but it currently returns the raw requested limit without enforcing `_MAX_LIMIT`. Update `extract_paging` to clamp the computed `limit` against `_MAX_LIMIT` (while preserving the existing default behavior), so all callers including `resolve_deviceHistoryGrouped` automatically get the cap; use the existing `_MAX_LIMIT` and `_DEFAULT_LIMIT` symbols in `graphql_helpers.py` to keep the behavior centralized.server/models/device_history_instance.py (1)
209-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or downgrade leftover debug logging.
These
mylog("none", ...)calls (including a dev-timetype(sort)dump) run unconditionally on every history query. "none" isn't used elsewhere in this file for diagnostics —prune_historyuses"verbose". Recommend removing these or downgrading to"trace"/"verbose"before merge.🤖 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 `@server/models/device_history_instance.py` around lines 209 - 213, The history query path in device_history_instance is still emitting unconditional debug logs via mylog("none", ...), including the type(sort) dump, which should not ship as-is. Remove these leftover diagnostics or downgrade them to the existing verbose/trace style used elsewhere in the file, and update the logging calls around the history query flow so they no longer run at the current level on every request.front/changeLogCore.php (3)
126-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover debug
console.log.🧹 Proposed fix
- console.log(query) - </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@front/changeLogCore.phpat line 126, Remove the leftover debug logging in
the query handling path by deleting the console.log call inside the relevant
change-log logic in changeLogCore.php. Locate the query-related code path around
the console.log(query) statement and ensure no temporary debug output remains in
the final implementation.</details> <!-- cr-comment:v1:ce9fadc8412e8fd6b08f6ca3 --> --- `247-286`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Dead code: unused GraphQL `query` in `populateFilters`.** The `query` variable (Lines 248-254) references a resolver named `allDeviceHistoryGrouped` that doesn't appear to exist (the actual resolver elsewhere in this file is `deviceHistoryGrouped`). It's built but never sent — `populateFilters` uses the REST endpoint at Line 265 instead. Remove the dead code. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@front/changeLogCore.phparound lines 247 - 286, The GraphQL query built at
the start of populateFilters is dead code because it is never used and
references the wrong resolver name. Remove the unused query block from
populateFilters, keeping the existing AJAX call to the filters endpoint and its
URL-building logic intact, and leave the rest of the filter population flow
unchanged.</details> <!-- cr-comment:v1:b72e143e6d5e51086c1e18d5 --> --- `296-300`: _🚀 Performance & Scalability_ | _🔵 Trivial_ | _⚡ Quick win_ **Potential duplicate initial data fetch on page load.** `initializeHistoryTable()` is called synchronously right after firing `populateFilters()`. Since DataTables with `serverSide: true` triggers its own `ajax` call on initialization, and `populateFilters()`'s async success handler (Line 283) also explicitly calls `.ajax.reload()` once filters arrive, the table's data is very likely fetched twice on every page load. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@front/changeLogCore.phparound lines 296 - 300, Potential duplicate data
fetch during page load: initChangeLog() calls initializeHistoryTable()
immediately after populateFilters(), but populateFilters() later triggers its
own table reload once filters are ready. Update initChangeLog() and/or
populateFilters() so the History table is initialized or reloaded only once per
load, using initializeHistoryTable() and the populateFilters() success callback
to coordinate the first ajax call.</details> <!-- cr-comment:v1:024329a79cadcd760d1fe2d4 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@front/changeLogCore.php:
- Around line 128-171: Update the AJAX handler in changeLogCore.php so both
failure paths clear the UI state and GraphQL errors are handled. In the $.ajax
success callback, check resp.errors before reading
resp.data.deviceHistoryGrouped; if present, treat it like a failure and return
empty results. In the error callback, after invoking callback with empty data,
also call hideSpinner() and hideChangeLogSkeleton() so the table overlay is
removed on request failure. Keep the existing success flow in the same AJAX
block that uses getSetting("API_TOKEN"), callback, hideSpinner, and
hideChangeLogSkeleton.- Around line 66-72:
devGUIDis being assigned without a declaration, which
creates an implicit global and can collide with other scripts loaded by
changeLog.phpanddeviceDetails.php. DeclaredevGUIDexplicitly in the
scope where it is used in the change-log request handling, and do the same for
urlinsidepopulateFilters()so both identifiers are local rather than
global.- Around line 195-200: The render callback for the device row is inserting
unescaped values returned by getDevDataByGuid into both the anchor href and link
text, creating an XSS risk. Update the render function to escape the devMac and
devName results before interpolating them into the HTML, not just the GUID
passed into getDevDataByGuid. Keep the existing escHtml usage for the data GUID
and apply the same escaping consistently to the returned values in this column.In
@front/deviceDetails.php:
- Around line 170-175: The History tab content is being initialized too early
because changeLogCore.php runs initChangeLog() on $(document).ready, causing
unnecessary GraphQL and /devices/history/filters requests on page load. Update
the device details flow so the History tab’s setup is deferred until the tab is
first opened by hooking initChangeLog() behind the tab’s first shown.bs.tab
event, and keep the existing changeLogCore.php logic reachable from that
trigger.
Outside diff comments:
In@server/models/device_history_instance.py:
- Around line 18-31: Both
_fetchalland_executecan leak the temp SQLite
connection whenexecute()raises, so update these helpers to always close the
connection even on failure. Useget_temp_db_connection()in
DeviceHistoryInstance, and wrap the query/commit path in a try/finally so
conn.close()runs regardless of success or exception; keep the existing return
values from_fetchalland_executeunchanged.- Around line 118-136:
get_total_group_countis missing thesearchfilter,
so its count can differ from the grouped history results. Update
device_history_instance.get_total_group_countto acceptsearchand include
it in the clause-building logic alongsidedevGUID,changedColumn, and
changedBy, then thread the samepaging["search"]argument through both calls
ingraphql_endpoint.pythat invokeget_total_group_count. Use the existing
get_grouped_historyandget_all_grouped_historyfiltering pattern as the
reference so the count matches the displayed groups.
Duplicate comments:
In@server/api_server/graphql_endpoint.py:
- Around line 78-158: The unified history resolver is forwarding the client’s
limitfromextract_pagingintoresolve_deviceHistoryGroupedwithout any
maximum cap, so large requests can still overwhelmDevicesHistoryInstance.
Updateextract_pagingto enforce the same upper bound used by
apply_common_pagination(capoptions.limitwith_MAX_LIMIT) so
resolve_deviceHistoryGrouped,get_grouped_history, and
get_all_grouped_historyall receive bounded pagination values consistently.In
@server/models/device_history_instance.py:
- Around line 196-223: _query_grouped in device_history_instance.py still
paginates after fetching all matching history rows, so move pagination into SQL
instead of slicing grouped_list in Python. Update _query_grouped to first select
the page of distinct group keys (timestamp, changedBy, devGUID) using the
existing filters from _build_clauses plus ORDER BY and LIMIT/OFFSET, then fetch
only the rows for those keys and build the grouped result. Preserve the current
changedColumn/search semantics and keep the public query methods that rely on
_query_grouped aligned with the new SQL-based pagination.
Nitpick comments:
In@front/changeLogCore.php:
- Line 126: Remove the leftover debug logging in the query handling path by
deleting the console.log call inside the relevant change-log logic in
changeLogCore.php. Locate the query-related code path around the
console.log(query) statement and ensure no temporary debug output remains in the
final implementation.- Around line 247-286: The GraphQL query built at the start of populateFilters
is dead code because it is never used and references the wrong resolver name.
Remove the unused query block from populateFilters, keeping the existing AJAX
call to the filters endpoint and its URL-building logic intact, and leave the
rest of the filter population flow unchanged.- Around line 296-300: Potential duplicate data fetch during page load:
initChangeLog() calls initializeHistoryTable() immediately after
populateFilters(), but populateFilters() later triggers its own table reload
once filters are ready. Update initChangeLog() and/or populateFilters() so the
History table is initialized or reloaded only once per load, using
initializeHistoryTable() and the populateFilters() success callback to
coordinate the first ajax call.In
@server/api_server/graphql_helpers.py:
- Around line 144-167:
extract_pagingis the shared pagination entry point,
but it currently returns the raw requested limit without enforcing_MAX_LIMIT.
Updateextract_pagingto clamp the computedlimitagainst_MAX_LIMIT
(while preserving the existing default behavior), so all callers including
resolve_deviceHistoryGroupedautomatically get the cap; use the existing
_MAX_LIMITand_DEFAULT_LIMITsymbols ingraphql_helpers.pyto keep the
behavior centralized.In
@server/models/device_history_instance.py:
- Around line 209-213: The history query path in device_history_instance is
still emitting unconditional debug logs via mylog("none", ...), including the
type(sort) dump, which should not ship as-is. Remove these leftover diagnostics
or downgrade them to the existing verbose/trace style used elsewhere in the
file, and update the logging calls around the history query flow so they no
longer run at the current level on every request.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `bcbc008e-614d-4199-8c2f-8e4076e33dae` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 35313feff4bff19cd40fb00d189818ea2fb84a84 and e0449bed8e69bdd2d6ac43b00a8a31d60d97d51c. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `docs/img/DEVICE_MANAGEMENT/device_presence.png` is excluded by `!**/*.png` </details> <details> <summary>📒 Files selected for processing (42)</summary> * `back/app.conf` * `docs/API_GRAPHQL.md` * `docs/PERFORMANCE.md` * `docs/WHERE_NETALERTX_FITS.md` * `docs/index.md` * `front/changeLog.php` * `front/changeLogCore.php` * `front/css/app.css` * `front/deviceDetails.php` * `front/js/cache.js` * `front/php/templates/header.php` * `front/php/templates/language/ar_ar.json` * `front/php/templates/language/ca_ca.json` * `front/php/templates/language/cs_cz.json` * `front/php/templates/language/de_de.json` * `front/php/templates/language/en_us.json` * `front/php/templates/language/es_es.json` * `front/php/templates/language/fa_fa.json` * `front/php/templates/language/fi_fi.json` * `front/php/templates/language/fr_fr.json` * `front/php/templates/language/he_il.json` * `front/php/templates/language/id_id.json` * `front/php/templates/language/it_it.json` * `front/php/templates/language/ja_jp.json` * `front/php/templates/language/nb_no.json` * `front/php/templates/language/pl_pl.json` * `front/php/templates/language/pt_br.json` * `front/php/templates/language/pt_pt.json` * `front/php/templates/language/ru_ru.json` * `front/php/templates/language/sv_sv.json` * `front/php/templates/language/tr_tr.json` * `front/php/templates/language/uk_ua.json` * `front/php/templates/language/vi_vn.json` * `front/php/templates/language/zh_cn.json` * `front/php/templates/skel_change_log.php` * `front/pluginsCore.php` * `server/api_server/api_server_start.py` * `server/api_server/graphql_endpoint.py` * `server/api_server/graphql_helpers.py` * `server/api_server/graphql_types.py` * `server/initialise.py` * `server/models/device_history_instance.py` </details> <details> <summary>✅ Files skipped from review due to trivial changes (13)</summary> * front/php/templates/skel_change_log.php * front/pluginsCore.php * docs/index.md * front/php/templates/language/pl_pl.json * docs/PERFORMANCE.md * front/php/templates/language/fa_fa.json * front/php/templates/language/es_es.json * front/php/templates/language/nb_no.json * front/php/templates/language/pt_br.json * front/php/templates/language/ar_ar.json * front/php/templates/language/fi_fi.json * front/php/templates/language/ru_ru.json * docs/API_GRAPHQL.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (5)</summary> * back/app.conf * front/php/templates/header.php * server/api_server/api_server_start.py * server/api_server/graphql_types.py * server/initialise.py </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
front/changeLogCore.php (2)
256-269: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winStored XSS:
devMac/devNamestill interpolated unescaped.Same issue flagged previously:
escHtml()is applied only toguid(used as the lookup key), not to themac/namevalues returned bygetDevDataByGuid. Both are interpolated directly into the anchor'shrefand text without escaping — a stored XSS vector since device names are user-editable.🔒️ Proposed fix
targets: 2, render: function (data) { const guid = escHtml(data); - const mac = getDevDataByGuid(guid, "devMac"); - const name = getDevDataByGuid(guid, "devName"); + const mac = getDevDataByGuid(data, "devMac") || ""; + const name = getDevDataByGuid(data, "devName") || ""; return ` - <a href="deviceDetails.php?mac=${mac}" data-guid="${guid}"> - ${name} (${guid}) + <a href="deviceDetails.php?mac=${encodeURIComponent(mac)}" data-guid="${guid}"> + ${escHtml(name)} (${guid}) </a>`; }🤖 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 `@front/changeLogCore.php` around lines 256 - 269, The device link renderer in changeLogCore.php still interpolates unescaped values from getDevDataByGuid for devMac and devName, creating a stored XSS risk. Update the render function to escape both the href parameter and the displayed name before building the anchor, while keeping escHtml on the guid lookup key. Use the existing getDevDataByGuid and escHtml flow in the table renderer to locate and fix the unsafe interpolation.
193-233: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAJAX error path still leaves spinner/skeleton stuck; GraphQL
errorsstill unchecked.This still has the two gaps flagged in a previous review:
error:(lines 207-233) never callshideSpinner()/hideChangeLogSkeleton(), so a failed request leaves the overlay/spinner visible indefinitely.success:(lines 193-205) never inspectsresp.errors— GraphQL can return HTTP 200 with a top-levelerrorsarray and null/partialdata, which would silently render an empty table.🔧 Proposed fix
success: function (resp) { + if (resp?.errors?.length) { + console.error("History GraphQL error:", resp.errors); + } const result = resp?.data?.deviceHistoryGrouped; callback({ data: result?.history || [], recordsTotal: result?.count || 0, recordsFiltered: result?.count || 0 }); hideSpinner(); hideChangeLogSkeleton(); }, ... callback({ data: [], recordsTotal: 0, recordsFiltered: 0 }); + hideSpinner(); + hideChangeLogSkeleton(); }🤖 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 `@front/changeLogCore.php` around lines 193 - 233, In the AJAX flow inside the changelog request handler, make sure both the success and error paths fully clear the loading UI. Update the success callback to inspect resp.errors before using resp.data.deviceHistoryGrouped, and if GraphQL returned errors, log them and treat the response as failed/empty. Also add hideSpinner() and hideChangeLogSkeleton() to the error callback so the spinner/skeleton are always dismissed even when the request fails.
🤖 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 `@front/changeLogCore.php`:
- Around line 313-324: The URL built in populateFilters is malformed because
apiEndpoint already includes a leading slash and the current
`${apiBaseUrl}/${apiEndpoint}` concatenation creates a double slash, while
`encodeURIComponent(queryString)` wrongly encodes the whole query string and
hides `devGUID` from the server. Update the URL construction in populateFilters
so the base path and endpoint are joined without duplicating slashes, and only
encode the query parameter value returned by getDevDataByMac(urlMac, "devGUID")
rather than the entire query string. Use populateFilters, getApiBase, and
getDevDataByMac as the key symbols when adjusting this logic.
---
Duplicate comments:
In `@front/changeLogCore.php`:
- Around line 256-269: The device link renderer in changeLogCore.php still
interpolates unescaped values from getDevDataByGuid for devMac and devName,
creating a stored XSS risk. Update the render function to escape both the href
parameter and the displayed name before building the anchor, while keeping
escHtml on the guid lookup key. Use the existing getDevDataByGuid and escHtml
flow in the table renderer to locate and fix the unsafe interpolation.
- Around line 193-233: In the AJAX flow inside the changelog request handler,
make sure both the success and error paths fully clear the loading UI. Update
the success callback to inspect resp.errors before using
resp.data.deviceHistoryGrouped, and if GraphQL returned errors, log them and
treat the response as failed/empty. Also add hideSpinner() and
hideChangeLogSkeleton() to the error callback so the spinner/skeleton are always
dismissed even when the request fails.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d87ad9b-cd28-427e-a9f0-d7661b026cd5
⛔ Files ignored due to path filters (1)
docs/img/DEVICE_MANAGEMENT/device_change_log.pngis excluded by!**/*.png
📒 Files selected for processing (14)
docs/DEVICE_CHANGE_LOG.mddocs/DEVICE_PRESENCE.mddocs/WHERE_NETALERTX_FITS.mddocs/stylesheets/custom.cssfront/changeLog.phpfront/changeLogCore.phpfront/css/app.cssfront/deviceDetailsEvents.phpfront/deviceDetailsPresence.phpfront/js/common.jsfront/js/ui_components.jsfront/php/templates/language/en_us.jsonfront/presence.phpmkdocs.yml
💤 Files with no reviewable changes (1)
- front/changeLog.php
✅ Files skipped from review due to trivial changes (6)
- docs/stylesheets/custom.css
- front/js/ui_components.js
- front/deviceDetailsPresence.php
- docs/DEVICE_PRESENCE.md
- front/php/templates/language/en_us.json
- docs/WHERE_NETALERTX_FITS.md
Comment on lines
+313
to
+324
| function populateFilters() { | ||
|
|
||
| const apiBaseUrl = getApiBase(); | ||
| const apiEndpoint = '/devices/history/filters'; | ||
|
|
||
| let queryString = ""; | ||
|
|
||
| if (urlMac) { | ||
| queryString += `?devGUID=${getDevDataByMac(urlMac, "devGUID")}`; | ||
| } | ||
|
|
||
| const url = `${apiBaseUrl}/${apiEndpoint}${encodeURIComponent(queryString)}`; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filters endpoint URL is malformed: double-slash and over-encoded query string.
Two issues in the URL construction:
apiEndpointalready starts with/('/devices/history/filters'), and it's concatenated as${apiBaseUrl}/${apiEndpoint}, producing a double slash.encodeURIComponent(queryString)encodes the entire query string, including?and=, turning?devGUID=abc123into%3FdevGUID%3Dabc123— the server will never seedevGUIDas a query parameter, so the device-scoped filter silently never applies.
🔧 Proposed fix
const apiBaseUrl = getApiBase();
const apiEndpoint = '/devices/history/filters';
- let queryString = "";
-
- if (urlMac) {
- queryString += `?devGUID=${getDevDataByMac(urlMac, "devGUID")}`;
- }
-
- const url = `${apiBaseUrl}/${apiEndpoint}${encodeURIComponent(queryString)}`;
+ let queryString = "";
+
+ if (urlMac) {
+ queryString = `?devGUID=${encodeURIComponent(getDevDataByMac(urlMac, "devGUID"))}`;
+ }
+
+ const url = `${apiBaseUrl}${apiEndpoint}${queryString}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function populateFilters() { | |
| const apiBaseUrl = getApiBase(); | |
| const apiEndpoint = '/devices/history/filters'; | |
| let queryString = ""; | |
| if (urlMac) { | |
| queryString += `?devGUID=${getDevDataByMac(urlMac, "devGUID")}`; | |
| } | |
| const url = `${apiBaseUrl}/${apiEndpoint}${encodeURIComponent(queryString)}`; | |
| function populateFilters() { | |
| const apiBaseUrl = getApiBase(); | |
| const apiEndpoint = '/devices/history/filters'; | |
| let queryString = ""; | |
| if (urlMac) { | |
| queryString = `?devGUID=${encodeURIComponent(getDevDataByMac(urlMac, "devGUID"))}`; | |
| } | |
| const url = `${apiBaseUrl}${apiEndpoint}${queryString}`; |
🤖 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 `@front/changeLogCore.php` around lines 313 - 324, The URL built in
populateFilters is malformed because apiEndpoint already includes a leading
slash and the current `${apiBaseUrl}/${apiEndpoint}` concatenation creates a
double slash, while `encodeURIComponent(queryString)` wrongly encodes the whole
query string and hides `devGUID` from the server. Update the URL construction in
populateFilters so the base path and endpoint are joined without duplicating
slashes, and only encode the query parameter value returned by
getDevDataByMac(urlMac, "devGUID") rather than the entire query string. Use
populateFilters, getApiBase, and getDevDataByMac as the key symbols when
adjusting this logic.
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 by CodeRabbit