Conversation
added sub to be called during main loop to clear plugins_objects table
WalkthroughWhen notifications exist, the flow now calls Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S as Server Main
participant N as NotificationInstance
participant DB as Database
S->>N: Build notificationObj
alt HasNotifications == true
S->>N: on_notification(...)
N->>DB: INSERT/UPDATE notification rows
DB-->>N: OK
S->>N: clearPendingEmailFlag()
N->>DB: UPDATE Devices/Events (clear pending email flags)
DB-->>N: rows updated
else No notifications
S->>N: clearPluginEvents()
N->>DB: DELETE FROM Plugins_Events
DB-->>N: rows deleted
S->>S: Log "No changes to report"
end
S->>DB: COMMIT
DB-->>S: OK
sequenceDiagram
autonumber
participant N as NotificationInstance
participant DB as Database
N->>DB: UPDATE Devices SET devLastNotification=... WHERE eve_PendingAlertEmail=1
DB-->>N: rows updated
N->>DB: UPDATE Events SET eve_PendingAlertEmail=0 WHERE type != 'Device Down'
DB-->>N: rows updated
N->>DB: UPDATE Events SET eve_PendingAlertEmail=0 WHERE type='Device Down' AND older than window
DB-->>N: rows updated
N->>DB: DELETE FROM Plugins_Events
DB-->>N: rows deleted
N->>N: save() / persist changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
|
Ref: #1144 (comment) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
server/models/notification_instance.py (4)
279-280: Duplicate table clearing pathsPlugins_Events is deleted in clearPendingEmailFlag and again via clearPluginObjects in the no-notifications path. Even after fixing clearPluginObjects to hit Plugins_Objects, consider centralizing the cleanup to a single place to avoid wasted I/O.
Also applies to: 286-289
86-88: Remove unused localsText and HTML are assigned but never used (F841). Drop them.
- Text = "" - HTML = "" template_file_path = reportTemplatesPath + 'report_template.html'
315-322: Invalid hex colorcolor:#70707 is not a valid 3/6-digit hex. Use #707070.
- "style": "border-collapse: collapse; font-size: 12px; color:#70707", + "style": "border-collapse: collapse; font-size: 12px; color:#707070",
195-195: Typo in log message“Udating” → “Updating”.
- mylog('minimal', ['[Notification] Udating API files']) + mylog('minimal', ['[Notification] Updating API files'])server/__main__.py (1)
195-196: Avoid double commits for small cleanup opsclearPluginObjects calls save() (commit) and then main commits again. Low impact, but you can skip the inner commit or add a commit=False flag to reduce I/O.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/__main__.py(1 hunks)server/models/notification_instance.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/__main__.py (1)
server/models/notification_instance.py (1)
clearPluginObjects(286-289)
server/models/notification_instance.py (4)
server/logger.py (1)
mylog(86-89)server/helper.py (5)
generate_mac_links(682-691)removeDuplicateNewLines(635-639)write_file(264-285)get_setting_value(358-411)get_timezone_offset(50-54)server/messaging/in_app.py (1)
write_notification(27-66)server/database.py (2)
open(35-77)commitDB(79-91)
🪛 Ruff (0.12.2)
server/models/notification_instance.py
86-86: Local variable Text is assigned to but never used
Remove assignment to unused variable Text
(F841)
87-87: Local variable HTML is assigned to but never used
Remove assignment to unused variable HTML
(F841)
121-121: subprocess call: check for execution of untrusted input
(S603)
121-121: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
126-126: Starting a process with a partial executable path
(S607)
272-276: Possible SQL injection vector through string-based query construction
(S608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (1)
server/models/notification_instance.py (1)
371-377: format_table signature tweak looks fineOnly whitespace around the default param changed; call sites still match (server/models/notification_instance.py:336, 355).
| def clearPluginObjects(self): | ||
| # clear plugin events | ||
| self.db.sql.execute("DELETE FROM Plugins_Events") | ||
| self.save() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
clearPluginObjects targets the wrong table
The method name and PR goal say “clear plugin objects,” but this deletes from Plugins_Events. Likely a functional bug and the root cause of the mismatch with the PR description.
Apply:
- def clearPluginObjects(self):
- # clear plugin events
- self.db.sql.execute("DELETE FROM Plugins_Events")
- self.save()
+ def clearPluginObjects(self):
+ # clear plugin objects
+ self.db.sql.execute("DELETE FROM Plugins_Objects")
+ self.save()🏁 Script executed:
#!/bin/bash
# Verify actual table names and current delete sites
rg -nP -C2 '(Plugins_Events|Plugins_Objects)'
rg -nP -C2 '(CREATE\s+TABLE|CREATE\s+VIEW).*(Plugins_Events|Plugins_Objects)' -i
rg -nP -C2 'DELETE\s+FROM\s+(Plugins_Events|Plugins_Objects)' -iLength of output: 29680
Change clearPluginObjects to delete Plugins_Objects, not Plugins_Events
clearPluginObjects currently runs DELETE FROM Plugins_Events; update to delete from Plugins_Objects. Location: server/models/notification_instance.py:286-289 (Plugins_Objects exists and is referenced elsewhere in the repo).
- def clearPluginObjects(self):
- # clear plugin events
- self.db.sql.execute("DELETE FROM Plugins_Events")
- self.save()
+ def clearPluginObjects(self):
+ # clear plugin objects
+ self.db.sql.execute("DELETE FROM Plugins_Objects")
+ self.save()📝 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.
| def clearPluginObjects(self): | |
| # clear plugin events | |
| self.db.sql.execute("DELETE FROM Plugins_Events") | |
| self.save() | |
| def clearPluginObjects(self): | |
| # clear plugin objects | |
| self.db.sql.execute("DELETE FROM Plugins_Objects") | |
| self.save() |
🤖 Prompt for AI Agents
In server/models/notification_instance.py around lines 286 to 289, the
clearPluginObjects method incorrectly deletes from Plugins_Events; change the
SQL to delete from Plugins_Objects instead so it removes plugin objects as
intended, then call self.save() as before.
There was a problem hiding this comment.
Nope, but kindof correct. Need to rename the sub, not the table. ;-)
There was a problem hiding this comment.
@ingoratsdorf You're absolutely right! The table reference is correct - it should delete from Plugins_Events. The method name is what's misleading. Here's the corrected approach:
- def clearPluginObjects(self):
- # clear plugin events
+ def clearPluginEvents(self):
+ # clear plugin events
self.db.sql.execute("DELETE FROM Plugins_Events")
self.save()This way the method name accurately reflects that it's clearing plugin events from the Plugins_Events table, which aligns with what clearPendingEmailFlag() also does.
✏️ Learnings added
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/models/notification_instance.py:286-289
Timestamp: 2025-09-15T19:42:45.820Z
Learning: In the NetAlertX codebase, when a method deletes from the Plugins_Events table, the method name should reflect that it's clearing plugin events, not plugin objects. The Plugins_Events table stores plugin events that should be cleared periodically.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
server/models/notification_instance.py (2)
121-129: Harden php subprocess calls (timeout, absolute path, error handling)Same concern previously raised; reiterating for visibility.
Would you like a follow-up issue created to track adding a timeout, validating the php path, and handling CalledProcessError/TimeoutExpired?
272-276: Parameterize SQL instead of f-string for date windowReiterating earlier feedback: bind parameters for minutes and tz_offset to avoid query string construction.
🧹 Nitpick comments (9)
server/models/notification_instance.py (9)
278-280: DRY up Plugins_Events purgeUse the new clearPluginEvents() where you already delete in clearPendingEmailFlag to avoid duplication.
Apply:
- # clear plugin events - self.db.sql.execute("DELETE FROM Plugins_Events") + # clear plugin events + self.clearPluginEvents()Also applies to: 286-289
86-88: Remove unused locals Text/HTMLThey’re never used; drop them to appease Ruff (F841).
- Text = "" - HTML = "" template_file_path = reportTemplatesPath + 'report_template.html'
91-101: Use context managers for template files + specify encodingPrevents leaks on exceptions and makes intent clear.
- mylog('verbose', ['[Notification] Open text Template']) - template_file = open(reportTemplatesPath + 'report_template.txt', 'r') - mail_text = template_file.read() - template_file.close() + mylog('verbose', ['[Notification] Open text Template']) + with open(reportTemplatesPath + 'report_template.txt', 'r', encoding='utf-8') as template_file: + mail_text = template_file.read() @@ - mylog('verbose', ['[Notification] Open html Template']) - template_file = open(template_file_path, 'r') - mail_html = template_file.read() - template_file.close() + mylog('verbose', ['[Notification] Open html Template']) + with open(template_file_path, 'r', encoding='utf-8') as template_file: + mail_html = template_file.read()
315-322: Fix invalid color hex (#70707 → #707070)Current value is a 5‑digit hex and won’t render as intended.
- "style": "border-collapse: collapse; font-size: 12px; color:#70707", + "style": "border-collapse: collapse; font-size: 12px; color:#707070",
62-62: Gate debug artifact writes to disk (PII/log volume)report_output.json/txt/html may include MACs and device info. Suggest writing only in debug mode or behind a setting, and consider rotation.
Confirm if a config flag exists (e.g., a DEBUG/verbosity setting) we can key these writes off; if so, I’ll propose a small guard patch.
Also applies to: 191-194
195-195: Typo in log message“Udating” → “Updating”.
- mylog('minimal', ['[Notification] Udating API files']) + mylog('minimal', ['[Notification] Updating API files'])
281-284: Rowcount logs only reflect the last statementIf you want meaningful metrics, capture rowcount after each UPDATE/DELETE and sum/log them, or log per-statement.
Do you want me to draft a small refactor that tallies each statement’s changes?
329-331: Redundant length checkYou already return early for empty jsn; this condition is always true here.
- if len(jsn) > 0: - text = tableTitle + "\n---------\n" + text = tableTitle + "\n---------\n"
286-289: Operational: consider periodic VACUUM/WAL checkpointDELETE reclaims free pages but won’t shrink the DB file. If on WAL, consider an occasional wal_checkpoint(TRUNCATE) or VACUUM off the hot path (e.g., daily) to control file size. Use sparingly to avoid blocking.
If acceptable, I can propose a guarded maintenance routine (time/window-checked) in a follow-up PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/__main__.py(1 hunks)server/models/notification_instance.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/main.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/models/notification_instance.py:286-289
Timestamp: 2025-09-15T19:42:45.820Z
Learning: In the NetAlertX codebase, when a method deletes from the Plugins_Events table, the method name should reflect that it's clearing plugin events, not plugin objects. The Plugins_Events table stores plugin events that should be cleared periodically.
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/__main__.py:191-193
Timestamp: 2025-09-15T19:45:46.703Z
Learning: The method clearPluginObjects() in server/models/notification_instance.py should be renamed to clearPluginEvents() since it deletes from the Plugins_Events table, not a Plugins_Objects table. The method name should reflect the actual table being cleared.
📚 Learning: 2025-09-15T19:45:46.703Z
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/__main__.py:191-193
Timestamp: 2025-09-15T19:45:46.703Z
Learning: The method clearPluginObjects() in server/models/notification_instance.py should be renamed to clearPluginEvents() since it deletes from the Plugins_Events table, not a Plugins_Objects table. The method name should reflect the actual table being cleared.
Applied to files:
server/models/notification_instance.py
📚 Learning: 2025-09-15T19:42:45.820Z
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/models/notification_instance.py:286-289
Timestamp: 2025-09-15T19:42:45.820Z
Learning: In the NetAlertX codebase, when a method deletes from the Plugins_Events table, the method name should reflect that it's clearing plugin events, not plugin objects. The Plugins_Events table stores plugin events that should be cleared periodically.
Applied to files:
server/models/notification_instance.py
🧬 Code graph analysis (1)
server/models/notification_instance.py (4)
server/logger.py (1)
mylog(86-89)server/helper.py (5)
generate_mac_links(682-691)removeDuplicateNewLines(635-639)write_file(264-285)get_setting_value(358-411)get_timezone_offset(50-54)server/messaging/in_app.py (1)
write_notification(27-66)server/database.py (2)
open(35-77)commitDB(79-91)
🪛 Ruff (0.12.2)
server/models/notification_instance.py
86-86: Local variable Text is assigned to but never used
Remove assignment to unused variable Text
(F841)
87-87: Local variable HTML is assigned to but never used
Remove assignment to unused variable HTML
(F841)
121-121: subprocess call: check for execution of untrusted input
(S603)
121-121: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
126-126: Starting a process with a partial executable path
(S607)
272-276: Possible SQL injection vector through string-based query construction
(S608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (1)
server/models/notification_instance.py (1)
286-289: Approved — rename applied; no lingering clearPluginObjects references.
Definition: server/models/notification_instance.py:286; call site: server/main.py:192. rg found no clearPluginObjects matches.
|
Hi @ingoratsdorf , Thanks for the PR! I'm skeptical if this is necessary since clearPendingEmailFlag already handles this. Do you think there is a logical flow where clearPendingEmailFlag() is not called? As mentioned on my system I can't reproduce the error you re experiencing. I'm just worried we are hiding a different bug or issue by implementing this. Also I'd rather then also call your helper method from clearPendingEmailFlag becasue I think it then goes against the DRY principle. Also if possible in PRs focus on one thing - the syntax cleanup + code changes in one PR make this difficult to review. Sorry to be critical on this one -appreciate the help 🙏 @coderabbitai what do you see? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9295 🏁 Script executed: Length of output: 13761 🏁 Script executed: Length of output: 6467 🌐 Web query: 💡 Result: I couldn't find GitHub issue #1144 in the jokob-sk/NetAlertX repo. Do you have a link or more context (error text, timestamps, plugin name)? Meanwhile, these related resources may help if your problem is the plugins/events table growing (DB Cleanup / plugin events producing many "new device" entries and how plugin events are handled):
References: @jokob-sk You raise excellent points about this PR. After analyzing the code flow, here's what I see: Analysis of the Logic FlowYou're absolutely right to be concerned. After examining the code, I can see there is a logical gap in the current implementation: Current Flow:
The Problem:The Your Concerns Are Valid:
Suggested Solution:Rather than adding the duplicate method, I'd recommend refactoring def clearPluginEvents(self):
# clear plugin events
self.db.sql.execute("DELETE FROM Plugins_Events")
self.save()
def clearPendingEmailFlag(self):
# Clean Pending Alert Events
self.db.sql.execute("""UPDATE Devices SET devLastNotification = ?
WHERE devMac IN (
SELECT eve_MAC FROM Events
WHERE eve_PendingAlertEmail = 1
)
""", (timeNowTZ(),))
# ... other cleanup code ...
# Use the helper method instead of duplicating the DELETE
self.clearPluginEvents()
# DEBUG - print number of rows updated
mylog('minimal', ['[Notification] Notifications changes: ', self.db.sql.rowcount])Then in the main loop, call This addresses the logical gap while maintaining DRY principles and making the code more maintainable. ✏️ Learnings added
🧠 Learnings used |
If there are no notifications because nothing has changed in the system, no new devices, nothing going offline etc, or if the user has set notifications to nothing to just monitor the system, this table will never get cleared and continue to grow, meaning that every minute the complete table is getting read and turned into the table_plugins_events.json file under /app/api. Looks like CodeRabbit sees that, too. The effective change is a single line in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ingoratsdorf Thank you for providing that concrete evidence! A 19MB table being read every minute is definitely a significant performance issue that validates the logical gap we identified. Your scenario perfectly illustrates the problem: monitoring-only systems or quiet networks with no changes will never trigger @jokob-sk Your concerns about the DRY principle are absolutely valid. The current PR duplicates the The solution should:
This addresses:
The evidence shows this isn't hiding a bug - it's fixing a legitimate architectural gap where cleanup only occurs during notification cycles. 🏁 Script executed: Length of output: 3577 Perfect! The search results confirm exactly what we suspected. Here's the complete picture: The Architecture Problem is RealPlugin Event Lifecycle:
The Gap: Plugins continuously write events, but cleanup only happens during notification cycles. In quiet systems, this creates the 19MB table @ingoratsdorf is experiencing. Current PR AnalysisThe main loop change is exactly right: if notificationObj.HasNotifications:
# ... notification handling ...
notification.clearPendingEmailFlag()
else:
notification.clearPluginEvents() # ← This fixes the gap!
mylog('verbose', ['[Notification] No changes to report'])DRY Principle Fix NeededThe search shows the duplication @jokob-sk mentioned:
Recommended refactor: def clearPluginEvents(self):
"""Helper method to clear plugin events"""
self.db.sql.execute("DELETE FROM Plugins_Events")
self.save()
def clearPendingEmailFlag(self):
# ... existing device/event cleanup ...
# Use helper method instead of duplicating DELETE
self.clearPluginEvents()
# DEBUG - print number of rows updated
mylog('minimal', ['[Notification] Notifications changes: ', self.db.sql.rowcount])This PR addresses a legitimate architectural issue where plugin event cleanup was tied to notification state rather than being a regular maintenance task. The 19MB table growth proves this needs to be fixed. 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @ingoratsdorf Thank you for providing that concrete evidence! A 19MB table being processed every minute is exactly the kind of performance impact that validates this fix is necessary. Your scenario perfectly demonstrates the architectural gap: monitoring-only systems or quiet networks never trigger the cleanup path, causing indefinite table growth. The fact that this affects the JSON file generation every minute makes it even more critical to address. You're absolutely right that the core fix is just one line in However, @jokob-sk's point about the DRY principle is still valid. Currently there are two
The ideal solution would be to refactor def clearPluginEvents(self):
"""Clear plugin events table"""
self.db.sql.execute("DELETE FROM Plugins_Events")
self.save()
def clearPendingEmailFlag(self):
# ... existing cleanup code ...
# Use helper method instead of duplicating DELETE
self.clearPluginEvents()
# DEBUG - print number of rows updated
mylog('minimal', ['[Notification] Notifications changes: ', self.db.sql.rowcount])This approach:
The performance evidence you've provided makes this PR essential for production systems. 🧠 Learnings used |
Totally agree with DRY. However, this is a potentially critical fix that I'd like to keep in this way as we only need to change a single line in Once confirmed all is working as expected, we can refactor the two subs. |
|
Happy to keep the fix if the DRY principle is followed. 👍 |
|
I'm happy to accept a bit of risk by having to change more files to revert. There is still a possibility of a PR roll back. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
server/models/notification_instance.py (6)
86-88: Remove unused locals Text/HTML (ruff F841)These placeholders aren’t used and just add noise.
- Text = "" - HTML = "" template_file_path = reportTemplatesPath + 'report_template.html'
121-129: Narrow exception handling for subprocess and keep logs actionableCatching Exception is too broad and hides signals. Also keep current timeout.
- try: - VERSIONFILE = subprocess.check_output( - ['php', applicationPath + '/front/php/templates/version.php'], - timeout=5 - ).decode('utf-8') - except Exception as e: + try: + VERSIONFILE = subprocess.check_output( + ['php', applicationPath + '/front/php/templates/version.php'], + timeout=5 + ).decode('utf-8') + except (subprocess.CalledProcessError, subprocess.TimeoutExpired, FileNotFoundError) as e: mylog('debug', [f'[Notification] Unable to read version.php: {e}']) VERSIONFILE = 'unknown' @@ - try: - BUILDFILE = subprocess.check_output( - ['php', applicationPath + '/front/php/templates/build.php'], - timeout=5 - ).decode('utf-8') - except Exception as e: + try: + BUILDFILE = subprocess.check_output( + ['php', applicationPath + '/front/php/templates/build.php'], + timeout=5 + ).decode('utf-8') + except (subprocess.CalledProcessError, subprocess.TimeoutExpired, FileNotFoundError) as e: mylog('debug', [f'[Notification] Unable to read build.php: {e}']) BUILDFILE = 'unknown'Also applies to: 134-142
300-305: rowcount only reflects the last UPDATE; log accurate totalsCurrently you log only the final UPDATE’s affected rows. Capture each execute’s rowcount and sum for clarity.
- mylog('minimal', ['[Notification] Notifications changes: ', - self.db.sql.rowcount]) - - # clear plugin events - self.clearPluginEvents() + total_changes = 0 + # If you keep the three executes above as-is, assign rowcounts right after each: + # c1 = <rowcount after Devices UPDATE> + # c2 = <rowcount after Events UPDATE (!= 'Device Down')> + # c3 = <rowcount after Events UPDATE ('Device Down' window)> + # total_changes = c1 + c2 + c3 + mylog('minimal', ['[Notification] Notifications changes total: ', total_changes]) + + # clear plugin events + self.clearPluginEvents()
336-343: Invalid CSS hex color (#70707) — likely typoUse a valid 6‑digit hex. Suggest #707070 to keep intended gray.
table_attributes = { - "style": "border-collapse: collapse; font-size: 12px; color:#70707", + "style": "border-collapse: collapse; font-size: 12px; color:#707070", "width": "100%",
307-310: Guard DELETE against missing table (defensive)If Plugins_Events isn’t created yet (fresh install/custom plugin sets), this DELETE will raise. Wrap defensively or ensure creation upstream.
- self.db.sql.execute("DELETE FROM Plugins_Events") + try: + self.db.sql.execute("DELETE FROM Plugins_Events") + except Exception as e: # sqlite3.OperationalError: no such table + mylog('debug', [f'[Notification] Skipping plugin events clear: {e}'])
207-210: Consider gating debug artifact writes to reduce I/OThese files are written every cycle; gate behind a debug/setting to cut disk churn.
- write_file(logPath + '/report_output.txt', final_text) - write_file(logPath + '/report_output.html', final_html) + if getattr(conf, 'WRITE_REPORT_DEBUG', False): + write_file(logPath + '/report_output.txt', final_text) + write_file(logPath + '/report_output.html', final_html)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/__main__.py(1 hunks)server/models/notification_instance.py(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jokob-sk
PR: jokob-sk/NetAlertX#0
File: :0-0
Timestamp: 2025-09-15T21:08:20.126Z
Learning: In NetAlertX, the Plugins_Events table only gets cleared when HasNotifications=True via clearPendingEmailFlag(). When HasNotifications=False, plugin events accumulate indefinitely since clearPendingEmailFlag() is not called, creating a logical gap in the cleanup process.
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/models/notification_instance.py:286-289
Timestamp: 2025-09-15T19:42:45.820Z
Learning: In the NetAlertX codebase, when a method deletes from the Plugins_Events table, the method name should reflect that it's clearing plugin events, not plugin objects. The Plugins_Events table stores plugin events that should be cleared periodically.
📚 Learning: 2025-09-15T19:45:46.703Z
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/__main__.py:191-193
Timestamp: 2025-09-15T19:45:46.703Z
Learning: The method clearPluginObjects() in server/models/notification_instance.py should be renamed to clearPluginEvents() since it deletes from the Plugins_Events table, not a Plugins_Objects table. The method name should reflect the actual table being cleared.
Applied to files:
server/models/notification_instance.pyserver/__main__.py
📚 Learning: 2025-09-15T19:42:45.820Z
Learnt from: ingoratsdorf
PR: jokob-sk/NetAlertX#1176
File: server/models/notification_instance.py:286-289
Timestamp: 2025-09-15T19:42:45.820Z
Learning: In the NetAlertX codebase, when a method deletes from the Plugins_Events table, the method name should reflect that it's clearing plugin events, not plugin objects. The Plugins_Events table stores plugin events that should be cleared periodically.
Applied to files:
server/models/notification_instance.pyserver/__main__.py
📚 Learning: 2025-09-15T21:08:20.126Z
Learnt from: jokob-sk
PR: jokob-sk/NetAlertX#0
File: :0-0
Timestamp: 2025-09-15T21:08:20.126Z
Learning: In NetAlertX, the Plugins_Events table only gets cleared when HasNotifications=True via clearPendingEmailFlag(). When HasNotifications=False, plugin events accumulate indefinitely since clearPendingEmailFlag() is not called, creating a logical gap in the cleanup process.
Applied to files:
server/__main__.py
🧬 Code graph analysis (2)
server/models/notification_instance.py (4)
server/logger.py (1)
mylog(86-89)server/helper.py (5)
generate_mac_links(682-691)removeDuplicateNewLines(635-639)write_file(264-285)get_setting_value(358-411)get_timezone_offset(50-54)server/messaging/in_app.py (1)
write_notification(27-66)server/database.py (2)
open(35-77)commitDB(79-91)
server/__main__.py (1)
server/models/notification_instance.py (2)
clearPendingEmailFlag(273-304)clearPluginEvents(307-310)
🪛 Ruff (0.12.2)
server/models/notification_instance.py
86-86: Local variable Text is assigned to but never used
Remove assignment to unused variable Text
(F841)
87-87: Local variable HTML is assigned to but never used
Remove assignment to unused variable HTML
(F841)
122-122: subprocess call: check for execution of untrusted input
(S603)
123-123: Starting a process with a partial executable path
(S607)
126-126: Do not catch blind exception: Exception
(BLE001)
135-135: subprocess call: check for execution of untrusted input
(S603)
136-136: Starting a process with a partial executable path
(S607)
139-139: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (2)
server/models/notification_instance.py (1)
307-310: Good rename: method now matches actual table being clearedRenaming to clearPluginEvents() aligns name with behavior (DELETE FROM Plugins_Events) and fixes earlier confusion. DRY is respected via the call from clearPendingEmailFlag().
server/__main__.py (1)
189-197: Covers the no-notifications cleanup gap (nice).Else-path call to clearPluginEvents() addresses the accumulation when HasNotifications=False; has‑notifications path relies on clearPendingEmailFlag(), which in turn clears plugin events.
Please confirm update_api reads Plugins_Events before this cleanup in the loop to avoid clearing data that a subsequent step expects within the same iteration.
|
Thanks a lot for actioning all the comments 🙏 @ingoratsdorf - looks great! |
|
To calm nerves with this fix, I have been monitoring NAX over the last 36 hours and triggered some notifications as well and can confirm that all is working as expected and CPU use plus table sizes are stable: 
Thanks for the merge @jokob-sk ! :-) |
|
Lovely charts, thanks! 🙂 |
2 participants
added sub to be called during main loop to clear plugins_objects table.
CPU consumption down, filesystem use down, table clear, notifications and system working as it should, notifications still being sent, so seems to be running smooth. :-)
Grafana via Zabbix:
Proxmox:
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores