Next release#1634
Conversation
… message Signed-off-by: jokob-sk <jokob.sk@gmail.com>
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCentralizes safe trigger/object handling and makes Action.execute() parameterless; updates action subclasses and orchestration; refactors device page title resolution to async with cache retry and REST fallback; adjusts DIGSCAN scheduling and plugin default; replaces upgrade emojis with Font Awesome icons; logging, ICMP scan, timezone, and README/config tweaks. ChangesWorkflow Action Execution Refactoring
Notification UI Icon Update
Device Page Title & Navigation
README Badge Update
DIGSCAN Configuration
ICMP Scan Update
Sync Endpoint Logging
Session Timestamps
Sync Plugin Config
API Server Start Formatting
Messaging / Reporting Timezone
Datetime Utils
Sequence Diagram(s) sequenceDiagram
participant Trigger
participant ActionGroup
participant UpdateFieldAction
participant DeleteObjectAction
participant RunPluginAction
participant SendNotificationAction
participant DB
Trigger->>ActionGroup: trigger fires
ActionGroup->>UpdateFieldAction: action.execute()
UpdateFieldAction->>DB: Action.get_object() -> dict or None
alt object present
UpdateFieldAction->>DB: update by objectGuid/devGUID
DeleteObjectAction->>DB: delete by objectGuid/devGUID
RunPluginAction->>RunPluginAction: execute plugin with object
SendNotificationAction->>SendNotificationAction: log/emit notification (when supported)
else missing
UpdateFieldAction-->>ActionGroup: early return / log
end
ActionGroup-->>Trigger: complete
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 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 `@server/workflows/actions.py`:
- Around line 105-107: When the code logs "[WF] Unsupported object format:
{obj}" it should follow the same early-return pattern used in UpdateFieldAction
and return None instead of returning the original obj; update the
function/method that contains the mylog call (the block that currently does
mylog("none", f"[WF] Unsupported object format: {obj}") followed by return obj)
to return None so callers get a clear failure signal for unsupported formats.
- Around line 70-72: The function logs unsupported object formats with mylog but
then returns the original obj, which is inconsistent with the earlier
None-return pattern; modify the branch that detects missing objectGuid and
devGUID (the unsupported-format case) to return None instead of obj so callers
receive a clear failure signal; update any docstring/comments for the enclosing
function (in server/workflows/actions.py) if present to reflect that unsupported
formats return None and run tests that call this function to ensure callers
handle None appropriately.
🪄 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: 8e7d1402-381e-4f54-a7d8-294fa1901f80
📒 Files selected for processing (3)
server/initialise.pyserver/workflows/actions.pyserver/workflows/triggers.py
- Updated Trendshift repository link in README.md. - Improved error handling and user feedback in deviceDetails.php: - Added warnings for device not found scenarios. - Refactored device name and owner retrieval logic with caching and REST API fallback.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
…fine device management logic in deviceDetails.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/dig_scan/config.json`:
- Line 55: The upgrade will cause existing installs without an explicit
DIGSCAN_RUN in their app.conf to inherit the new config.json default
("disabled") because initialiseFile() only creates a template when the file is
missing; to fix, add a migration or merge step that runs at startup (or enhance
initialiseFile()) to read the existing app.conf and, if DIGSCAN_RUN is absent,
set and persist DIGSCAN_RUN='before_name_updates' (the new intended default from
back/app.conf) so upgrades keep the new behavior, or alternatively surface a
clear upgrade notice to users to set DIGSCAN_RUN explicitly; reference the
symbols: DIGSCAN_RUN, initialiseFile(), config.json (default_value), and
back/app.conf (intended default).
🪄 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: c6185fd5-c24a-46d8-a963-5b319dc79d4f
📒 Files selected for processing (3)
back/app.conffront/plugins/dig_scan/config.jsonserver/initialise.py
| ] | ||
| }, | ||
| "default_value": "before_name_updates", | ||
| "default_value": "disabled", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how plugin defaults are initialized and if there's migration for this change
# Look for default value handling in the plugin initialization
rg -nP -C5 'default_value.*before_name_updates|DIGSCAN_RUN' --type=py
# Check for any upgrade migration logic that might handle this change
ast-grep --pattern 'def $FUNC($$$) {
$$$
DIGSCAN
$$$
}'
# Look for how settings from app.conf override plugin defaults
rg -nP -C3 'def (importConfigs|ccd|get_set_value)' server/initialise.pyRepository: netalertx/NetAlertX
Length of output: 1188
🏁 Script executed:
# Examine the config.json file to verify the default_value at line 55
cat -n front/plugins/dig_scan/config.json | head -70Repository: netalertx/NetAlertX
Length of output: 2157
🏁 Script executed:
# Check back/app.conf for DIGSCAN_RUN setting at line 33
cat -n back/app.conf | sed -n '25,45p'Repository: netalertx/NetAlertX
Length of output: 969
🏁 Script executed:
# Find where DIGSCAN_RUN or the RUN setting is referenced to understand default handling
rg -n "RUN.*default" front/plugins/dig_scan/config.json -A 2 -B 2Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Understand how defaults are applied - examine the ccd() function and surrounding code
sed -n '58,120p' server/initialise.pyRepository: netalertx/NetAlertX
Length of output: 1761
🏁 Script executed:
# Check for upgrade/migration related code
rg -n "migrate|upgrade|DIGSCAN" server/initialise.py | head -20Repository: netalertx/NetAlertX
Length of output: 335
🏁 Script executed:
# Find where ccd() is called for DIGSCAN_RUN specifically
rg -n "DIGSCAN_RUN|ccd\(" server/initialise.py -B 2 -A 2 | head -50Repository: netalertx/NetAlertX
Length of output: 1382
🏁 Script executed:
# Understand how app.conf is loaded - search for where configuration files are parsed
rg -n "app\.conf|load.*config|parse.*config" server/initialise.py | head -20Repository: netalertx/NetAlertX
Length of output: 141
🏁 Script executed:
# Look at the importConfigs function more completely to understand the flow
sed -n '195,300p' server/initialise.pyRepository: netalertx/NetAlertX
Length of output: 4995
🏁 Script executed:
# Search for where plugin settings from config.json are initialized with defaults
rg -n "default_value" server/initialise.py -B 3 -A 3Repository: netalertx/NetAlertX
Length of output: 414
🏁 Script executed:
# Check if there's any special handling when loading app.conf for upgrades
rg -n "app\.conf" . --type=py | grep -i "upgrade\|migrate\|load"Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Find the read_config_file implementation
rg -n "def read_config_file" server/initialise.py -A 20Repository: netalertx/NetAlertX
Length of output: 1069
🏁 Script executed:
# Check how environment variables and app.conf are merged/loaded
rg -n "fullConfPath\|read_config_file" server/initialise.py -B 2 -A 2Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Look for any automatic migration that adds settings to app.conf on upgrade
rg -n "write.*app\.conf\|update.*config\|DIGSCAN_RUN" server/initialise.pyRepository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Check git history to see when the default changed
git log --oneline -- front/plugins/dig_scan/config.json back/app.conf | head -20Repository: netalertx/NetAlertX
Length of output: 150
🏁 Script executed:
# Search across the codebase for how DIGSCAN_RUN was used before
git log -p --all -S "DIGSCAN_RUN" -- front/plugins/dig_scan/config.json | head -100Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Get the full commit details for the DIGSCAN fix
git log --all -p -S "DIGSCAN could not" | head -200Repository: netalertx/NetAlertX
Length of output: 149
🏁 Script executed:
# Show what was changed in config.json related to DIGSCAN defaults
git show f11a63bb -- front/plugins/dig_scan/config.json | head -100Repository: netalertx/NetAlertX
Length of output: 2860
🏁 Script executed:
# Check if there's any other reference to issue `#1631`
rg -n "1631\|DIGSCAN.*disabled" . --type=py --type=md --type=json | head -20Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Look at the broader context - what was the default before?
git log --all --oneline -p -- front/plugins/dig_scan/config.json | grep -A 5 -B 5 "default_value" | head -50Repository: netalertx/NetAlertX
Length of output: 1355
🏁 Script executed:
# Check if there are any references to dig_scan config.json before the recent commit
git log --all --oneline -- front/plugins/dig_scan/config.json | head -10Repository: netalertx/NetAlertX
Length of output: 509
🏁 Script executed:
# Look for migration or initialization logic that might add DIGSCAN_RUN to app.conf
rg -n "renameSettings\|migrate\|add.*to.*conf\|write.*app\.conf" server/initialise.py -A 5 | head -60Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Check the renameColumnReferences function to understand migration patterns
sed -n '913,950p' server/initialise.pyRepository: netalertx/NetAlertX
Length of output: 1247
🏁 Script executed:
# Look for any logic that updates or adds missing settings to app.conf during upgrade
rg -n "def.*upgrade|upgrade.*config|config.*upgrade" server/ --type=py | head -20Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Check if there's template or initialization for app.conf that would include DIGSCAN_RUN
rg -n "app\.conf" . --type=py | grep -E "template|default|init" | head -15Repository: netalertx/NetAlertX
Length of output: 256
🏁 Script executed:
# Check git history for when DIGSCAN_RUN was added to app.conf
git log --all --oneline -p -- back/app.conf | grep -B 5 -A 5 "DIGSCAN_RUN" | head -40Repository: netalertx/NetAlertX
Length of output: 419
🏁 Script executed:
# Examine the initialiseFile function to understand how app.conf is set up
rg -n "def initialiseFile" server/helper.py -A 30Repository: netalertx/NetAlertX
Length of output: 1739
🏁 Script executed:
# Check if app.conf gets updated/merged with defaults during startup
rg -n "app\.conf.*write\|append.*app\.conf\|update.*app\.conf" server/ --type=py -B 2 -A 2Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Look at what the default app.conf template contains and when it's used
cat -n back/app.conf | head -50Repository: netalertx/NetAlertX
Length of output: 2314
🏁 Script executed:
# Check if there's any logic that compares app.conf with defaults and fills in missing settings
rg -n "missing.*setting|new.*setting|upgrade.*setting" server/initialise.py -B 2 -A 5Repository: netalertx/NetAlertX
Length of output: 397
Confirm the upgrade path for existing installations without explicit DIGSCAN_RUN configuration.
The change is correct in principle: new installations get DIGSCAN_RUN='before_name_updates' from back/app.conf line 33, and the config.json default is now disabled. However, existing installations that upgrade will be affected: if they don't already have DIGSCAN_RUN explicitly set in their app.conf, they will inherit the new disabled default from config.json since initialiseFile() only copies the template app.conf when the file is missing—it does not merge new settings into existing installations. Users should be notified of this change or a migration mechanism should be added to automatically set DIGSCAN_RUN='before_name_updates' for existing users on upgrade.
🤖 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/dig_scan/config.json` at line 55, The upgrade will cause
existing installs without an explicit DIGSCAN_RUN in their app.conf to inherit
the new config.json default ("disabled") because initialiseFile() only creates a
template when the file is missing; to fix, add a migration or merge step that
runs at startup (or enhance initialiseFile()) to read the existing app.conf and,
if DIGSCAN_RUN is absent, set and persist DIGSCAN_RUN='before_name_updates' (the
new intended default from back/app.conf) so upgrades keep the new behavior, or
alternatively surface a clear upgrade notice to users to set DIGSCAN_RUN
explicitly; reference the symbols: DIGSCAN_RUN, initialiseFile(), config.json
(default_value), and back/app.conf (intended default).
There was a problem hiding this comment.
@coderabbitai double check - existing installations with DIGSCAN enabled will have their own the DIGSCAN_RUN setting already specidfied in teh app.conf so this should be a non-issue - but challenge me if wrong
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DIGSCAN_RUN existed in app.conf before this PR (history)
git log --all --oneline -- back/app.conf | head -20
echo "---DIGSCAN_RUN in git history---"
git log --all -p -- back/app.conf | grep -B 5 -A 5 "DIGSCAN_RUN" | head -60
echo "---Current app.conf DIGSCAN lines---"
grep -n "DIGSCAN" back/app.conf
echo "---Check how plugin settings are written to app.conf---"
rg -n "def.*write.*conf\|def.*save.*conf\|def.*persist\|def.*update.*conf" server/ --type=py | head -20
echo "---Check if ccd() or plugin init writes settings to app.conf---"
rg -n "app\.conf.*write\|write.*app\.conf\|open.*app\.conf.*w" server/ --type=py -B 2 -A 5 | head -60
echo "---Check how settings from plugin config.json get persisted---"
rg -n "def ccd\|def saveConfParameter\|def writeSettingsToFile" server/initialise.py -A 20 | head -80
echo "---Look for savePlugin or importPlugin in the codebase---"
rg -n "def.*save.*plugin\|def.*import.*plugin\|writePlugin" server/ --type=py | head -20
Length of output: 1694
@jokob-sk, I dug into the git history of back/app.conf and the key finding is:
DIGSCAN_RUN was NOT present in back/app.conf in any prior commit — it is introduced for the first time in this very PR.
So your claim holds for users who explicitly configured and saved DIGSCAN settings through the UI (those writes would land in their app.conf). However, the edge case that remains is:
Users who had DIGSCAN running with the old
before_name_updatesdefault but never explicitly saved that setting through the UI — theirapp.confwould have noDIGSCAN_RUNentry. After the upgrade,initialiseFile()won't merge the newback/app.conftemplate entry into their existing file, so they'd fall back to the newconfig.jsondefault ofdisabled.
Since DIGSCAN is in the default LOADED_PLUGINS, this could affect a non-trivial number of passive users.
That said, this PR is explicitly fixing #1631 ("DIGSCAN could not be disabled"), and the intent of the change is clear. If the release notes or upgrade notice calls this out, it's a reasonable trade-off. If you'd prefer to be safe, the only mitigation would be a startup merge step that writes DIGSCAN_RUN='before_name_updates' into existing app.conf files that lack the key — but that's a non-trivial change.
🧠 Learnings used
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1435
File: front/plugins/dig_scan/config.json:85-87
Timestamp: 2026-01-20T23:55:35.174Z
Learning: JSON parsers ignore whitespace between tokens. A leading whitespace after the opening brace is valid and should not trigger a missing comma error. This applies to plugin config files like front/plugins/**/config.json and similar config.json files in plugin directories; only flag real syntax errors or missing commas when the JSON structure itself is invalid.
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1636
File: front/plugins/kea_api/config.json:332-332
Timestamp: 2026-05-10T21:47:05.302Z
Learning: When reviewing NetAlertX plugin configuration files (config.json) under front/plugins, use the exact elementOptions ordering property key "orderable". Do not flag "orderable" as a misspelling and do not suggest changing it to "ordeable"; this spelling is intentionally used consistently across plugin config.json files (including templates and concrete plugins).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/api_server/sync_endpoint.py (1)
11-12: ⚡ Quick winAvoid caching
LOG_LEVELat module import time.Line 12 initializes
lggronce, so Line 35 can use stale verbosity after runtime setting changes. Build the logger (or read level) at call time for this check.♻️ Proposed fix
-# Make sure log level is initialized correctly -lggr = Logger(get_setting_value('LOG_LEVEL')) - def handle_sync_get(): @@ - if lggr.isAbove('verbose'): + if Logger(get_setting_value("LOG_LEVEL")).isAbove("verbose"): write_notification(message, 'info', timeNowUTC())Also applies to: 35-36
🤖 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/sync_endpoint.py` around lines 11 - 12, The module currently creates lggr = Logger(get_setting_value('LOG_LEVEL')) at import time which freezes the log level; remove that module-level initialization and instead build or read the level at call time—add a small accessor like get_logger() that returns Logger(get_setting_value('LOG_LEVEL')) (or call Logger(get_setting_value('LOG_LEVEL')) inline) and replace all uses of the imported lggr (including the runtime verbosity check that currently reads lggr around where you referenced lines 35-36) to call get_logger() or read get_setting_value('LOG_LEVEL') directly so the logger respects runtime changes.
🤖 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.
Nitpick comments:
In `@server/api_server/sync_endpoint.py`:
- Around line 11-12: The module currently creates lggr =
Logger(get_setting_value('LOG_LEVEL')) at import time which freezes the log
level; remove that module-level initialization and instead build or read the
level at call time—add a small accessor like get_logger() that returns
Logger(get_setting_value('LOG_LEVEL')) (or call
Logger(get_setting_value('LOG_LEVEL')) inline) and replace all uses of the
imported lggr (including the runtime verbosity check that currently reads lggr
around where you referenced lines 35-36) to call get_logger() or read
get_setting_value('LOG_LEVEL') directly so the logger respects runtime changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5675cbf-1953-45a3-a883-976b65b7b407
📒 Files selected for processing (1)
server/api_server/sync_endpoint.py
Translated using Weblate (Portuguese (Portugal))
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/icmp_scan/icmp.py`:
- Around line 216-236: The current loop uses a single is_ipv6 flag for the whole
batch, causing the "-6" option to be applied to IPv4 targets; change the logic
in the block that builds fping commands (the code around is_ipv6,
interface_list, for interface in interface_list, and cmd construction) to split
targets into two lists (ipv4_targets and ipv6_targets based on presence of ':'),
then iterate families: for each non-empty family list build cmd per interface
and only append "-6" when handling the ipv6_targets family; ensure you replace
uses of the single is_ipv6 variable with a per-invocation family check so IPv4
runs never include "-6" and IPv6 runs always include "-6".
🪄 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: ff468d7e-8488-47f5-9b15-9c7615ee3e7e
📒 Files selected for processing (2)
front/plugins/icmp_scan/icmp.pyserver/api_server/sync_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api_server/sync_endpoint.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/plugins/icmp_scan/icmp.py`:
- Around line 223-229: The loop iterates over interface_list built from
interfaces and will re-scan duplicate interface entries; deduplicate interfaces
before creating interface_list to avoid repeated scans (and preserve order).
Replace the current interface_list assignment logic (where interface_list =
interfaces if interfaces else [None]) with a deduplication step that builds a
list of unique interfaces (preserving first-seen order) and falls back to [None]
when interfaces is falsy, then iterate for interface in interface_list as
before.
- Around line 211-214: The code currently calls expand_subnets(targets)
unconditionally which can expand huge CIDRs into massive in-memory lists; before
expanding, compute an estimated host count for the provided CIDRs (using the
ipaddress module or a helper like count_hosts_in_subnets(targets)) and guard it
against a sensible MAX_TARGETS threshold (e.g., 65k) — if the estimate exceeds
the threshold, log/return an error or slice/refuse the request; alternatively
modify expand_subnets to accept a max_hosts parameter or return a
generator/streamed iterator to avoid allocating all hosts at once; update the
call site around expand_subnets and the subsequent handling of targets (the
targets variable and any fping invocation) to respect this guard.
🪄 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: 64ec046a-433e-485a-9f55-abe14cfa5a93
📒 Files selected for processing (2)
front/plugins/icmp_scan/icmp.pyserver/api_server/sync_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api_server/sync_endpoint.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/messaging/reporting.py`:
- Around line 229-234: The current except block around
apply_timezone_to_json(json_obj, section) drops the whole section on failure;
instead, catch the exception, log it (using mylog) and set data to the original
raw DB payload (json_obj or the variable that holds raw rows) so the section is
preserved, then continue to assign final_json[section] = data; update the except
block that references apply_timezone_to_json, mylog, json_obj, section and
final_json accordingly (remove the continue and fallback to raw data).
In `@server/utils/datetime_utils.py`:
- Around line 215-218: The current broad except Exception around constructing
target_tz can mask unrelated errors; narrow it to catch only
ZoneInfoNotFoundError and ValueError when calling ZoneInfo(str(conf.tz)) so that
conf.tz handling (involving conf.tz, ZoneInfo, ZoneInfoNotFoundError,
ValueError) falls back to datetime.UTC for unknown/invalid tz values while
allowing other exceptions to surface; update the except clause accordingly and
keep assignment to target_tz and the UTC fallback intact.
🪄 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: 15bfbc22-9b65-4a92-92b8-5e53053fb3cd
📒 Files selected for processing (5)
front/deviceDetailsSessions.phpfront/plugins/sync/config.jsonserver/api_server/api_server_start.pyserver/messaging/reporting.pyserver/utils/datetime_utils.py
💤 Files with no reviewable changes (2)
- front/deviceDetailsSessions.php
- server/api_server/api_server_start.py
| try: | ||
| target_tz = conf.tz if isinstance(conf.tz, datetime.tzinfo) else ZoneInfo(str(conf.tz)) | ||
| except Exception: | ||
| target_tz = datetime.UTC |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify broad exception handling and related BLE001 occurrences.
rg -n -C2 'except Exception' server/utils/datetime_utils.py
rg -n -C2 'format_date_iso\(' server/utils/datetime_utils.pyRepository: netalertx/NetAlertX
Length of output: 1209
🏁 Script executed:
head -50 server/utils/datetime_utils.pyRepository: netalertx/NetAlertX
Length of output: 1812
🏁 Script executed:
sed -n '200,230p' server/utils/datetime_utils.pyRepository: netalertx/NetAlertX
Length of output: 1157
🏁 Script executed:
rg -n 'import.*ZoneInfo' server/utils/datetime_utils.pyRepository: netalertx/NetAlertX
Length of output: 118
🏁 Script executed:
rg -n 'from.*ZoneInfo' server/utils/datetime_utils.pyRepository: netalertx/NetAlertX
Length of output: 118
🌐 Web query:
ZoneInfo constructor exceptions raised Python zoneinfo
💡 Result:
The Python zoneinfo.ZoneInfo constructor raises specific exceptions depending on the nature of the failure during object instantiation [1][2]. Primary Exceptions: - ZoneInfoNotFoundError: This exception is raised when the specified time zone key cannot be found on the system or within the available tzdata package [1][2]. It is a subclass of KeyError [1][2]. - ValueError: This exception is raised if the provided key is not a valid, normalized relative POSIX path (e.g., it contains up-level references like '..' or is otherwise non-conforming) [1][2]. Additionally, if using the ZoneInfo.from_file constructor, ValueError is raised if the data read from the file is not a valid TZif file [1][2]. Related Warnings and Other Errors: - InvalidTZPathWarning: This is a warning (not an exception) raised when the PYTHONTZPATH environment variable contains invalid components, such as relative paths, which are subsequently filtered out [1][2][3]. - TypeError: May be raised if invalid types are passed to configuration methods, such as when setting the time zone path [3]. - pickle.PicklingError: Raised if you attempt to pickle a ZoneInfo object that was constructed directly from a file-like object using ZoneInfo.from_file [1][2][4]. Best Practices: To handle these issues, it is recommended to validate keys against zoneinfo.available_timezones before instantiation [5][6]. For environments where system time zone data is missing (such as some Windows installations or minimal containers), ensure the tzdata package is installed as a fallback [2][7]. Always prefer the standard ZoneInfo(key) constructor over alternate constructors like no_cache unless specifically required for testing or cache invalidation scenarios [8].
Citations:
- 1: https://docs.python.org/3.15/library/zoneinfo.html
- 2: https://docs.python.org/3/library/zoneinfo.html
- 3: https://github.com/python/cpython/blob/main/Lib/zoneinfo/_tzpath.py
- 4: https://github.com/python/cpython/blob/master/Lib/zoneinfo/_zoneinfo.py
- 5: https://runebook.dev/en/docs/python/library/zoneinfo/exceptions-and-warnings
- 6: https://runebook.dev/en/docs/python/library/zoneinfo/zoneinfo.ZoneInfo.key
- 7: https://runebook.dev/en/docs/python/library/zoneinfo/data-sources
- 8: https://runebook.dev/en/docs/python/library/zoneinfo/zoneinfo.ZoneInfo.no_cache
Narrow the exception handling to only expected timezone errors.
The broad except Exception at line 217 can mask unrelated bugs; catching specific exceptions that ZoneInfo() raises is safer. Since ZoneInfoNotFoundError is already imported in the file, replace the catch with:
- except Exception:
+ except (ZoneInfoNotFoundError, ValueError):
target_tz = datetime.UTCZoneInfoNotFoundError is raised when the timezone key is not found, and ValueError is raised when the key is not a valid timezone path. This keeps the UTC fallback while preventing silent masking of unexpected errors.
📝 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.
| try: | |
| target_tz = conf.tz if isinstance(conf.tz, datetime.tzinfo) else ZoneInfo(str(conf.tz)) | |
| except Exception: | |
| target_tz = datetime.UTC | |
| try: | |
| target_tz = conf.tz if isinstance(conf.tz, datetime.tzinfo) else ZoneInfo(str(conf.tz)) | |
| except (ZoneInfoNotFoundError, ValueError): | |
| target_tz = datetime.UTC |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 217-217: Do not catch blind exception: Exception
(BLE001)
🤖 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/utils/datetime_utils.py` around lines 215 - 218, The current broad
except Exception around constructing target_tz can mask unrelated errors; narrow
it to catch only ZoneInfoNotFoundError and ValueError when calling
ZoneInfo(str(conf.tz)) so that conf.tz handling (involving conf.tz, ZoneInfo,
ZoneInfoNotFoundError, ValueError) falls back to datetime.UTC for
unknown/invalid tz values while allowing other exceptions to surface; update the
except clause accordingly and keep assignment to target_tz and the UTC fallback
intact.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/messaging/reporting.py (1)
227-250:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
NameErrorwhen DB query fails:json_objis undefined at line 242.If
db.get_table_as_json()raises an exception,json_objis never assigned. The inner try-except (lines 234-237) catches this when accessingjson_obj.json, but line 242 (getattr(json_obj, "columnNames", [])) is outside that block and will raiseNameError, crashing the entire notification process.Separating the DB fetch from timezone conversion (as suggested in the prior review) cleanly resolves both concerns:
🐛 Proposed fix
- try: - json_obj = db.get_table_as_json(sqlQuery, parameters) - data = apply_timezone_to_json(json_obj, section) - except Exception as e: - mylog("none", [f"[Notification] apply_timezone failed for section {section}: ", e]) - - # fallback: preserve raw DB payload instead of dropping section - try: - data = json_obj.json.get("data", []) - except Exception: - data = [] - - final_json[section] = data - final_json[f"{section}_meta"] = { - "title": SECTION_TITLES.get(section, section), - "columnNames": getattr(json_obj, "columnNames", []) - } - continue - - final_json[section] = data + try: + json_obj = db.get_table_as_json(sqlQuery, parameters) + except Exception as e: + mylog("none", [f"[Notification] DB error in section {section}: ", e]) + continue + + raw_data = json_obj.json.get("data", []) + try: + data = apply_timezone_to_json(json_obj, section) + except Exception as e: + mylog("none", [f"[Notification] Timezone conversion failed for section {section}: ", e]) + data = raw_data + + final_json[section] = data🤖 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/messaging/reporting.py` around lines 227 - 250, The except block for apply_timezone_to_json can reference json_obj even when db.get_table_as_json failed, causing a NameError; modify the try/except flow so db.get_table_as_json(...) is called in its own try and sets a safe default (e.g., json_obj = None or an empty structure) before calling apply_timezone_to_json(section), then in the outer exception handling use guarded access to json_obj (check if json_obj is truthy before using json_obj.json or getattr(json_obj, "columnNames", [])) and populate final_json[section] and final_json[f"{section}_meta"] using SECTION_TITLES and the safe columnNames fallback so the failure of db.get_table_as_json or apply_timezone_to_json does not raise another exception.
🤖 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.
Outside diff comments:
In `@server/messaging/reporting.py`:
- Around line 227-250: The except block for apply_timezone_to_json can reference
json_obj even when db.get_table_as_json failed, causing a NameError; modify the
try/except flow so db.get_table_as_json(...) is called in its own try and sets a
safe default (e.g., json_obj = None or an empty structure) before calling
apply_timezone_to_json(section), then in the outer exception handling use
guarded access to json_obj (check if json_obj is truthy before using
json_obj.json or getattr(json_obj, "columnNames", [])) and populate
final_json[section] and final_json[f"{section}_meta"] using SECTION_TITLES and
the safe columnNames fallback so the failure of db.get_table_as_json or
apply_timezone_to_json does not raise another exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ab845f9-97ce-4c8c-bd8a-ddfd658c7351
📒 Files selected for processing (1)
server/messaging/reporting.py
…pse, Dropdown and Modal
Fix potentially vulnerable cloned function
Fix potentially vulnerable cloned function
Fix potentially vulnerable cloned function
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/lib/moment/moment.js`:
- Line 2297: The vendored code uses the dotAll `s` flag in the regex on the
`s.replace(...)` line which breaks older engines; update the pattern to avoid
`s` by replacing `.` with `[\s\S]` and remove the `s` flag (e.g. change the
subpattern `(?:(?!\().)*` to `(?:(?!\()[\s\S])*`) so the full call to
`s.replace(...)` uses only `g` and no `s` flag while keeping the other
replacements (`.replace(/(\s\s+)/g, ' ')`, `.replace(/^\s\s*/, '')`,
`.replace(/\s\s*$/, '')`) unchanged.
🪄 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: 28c025e5-6a93-4427-82bc-a0290f0d0506
📒 Files selected for processing (1)
front/lib/moment/moment.js
| function preprocessRFC2822(s) { | ||
| // Remove comments and folding whitespace and replace multiple-spaces with a single space | ||
| return s.replace(/\([^)]*\)|[\n\t]/g, ' ').replace(/(\s\s+)/g, ' ').replace(/^\s\s*/, '').replace(/\s\s*$/, ''); | ||
| return s.replace(/\((?:(?!\().)*\)|[\n\t]/gs, ' ').replace(/(\s\s+)/g, ' ').replace(/^\s\s*/, '').replace(/\s\s*$/, ''); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether dotAll regex flag is used elsewhere in vendored JS (potential compatibility risk).
rg -nP '/[^/\n]*/[gimsuy]*s[gimsuy]*' front/lib | head -100Repository: netalertx/NetAlertX
Length of output: 50375
🏁 Script executed:
# Check the file size and read the specific lines around line 2297
wc -l front/lib/moment/moment.js
sed -n '2295,2300p' front/lib/moment/moment.jsRepository: netalertx/NetAlertX
Length of output: 426
🏁 Script executed:
# Check for browser compatibility configuration
fd -type f '(package.json|.browserslistrc|.babelrc|tsconfig|webpack)' front | head -20
cat package.json | grep -A 10 -B 2 'browserslist\|targets\|engines'Repository: netalertx/NetAlertX
Length of output: 279
🏁 Script executed:
# Find package.json and configuration files
git ls-files | grep -E '(package\.json|\.browserslistrc|\.babelrc|webpack|tsconfig)' | head -20
# Also check if there's any browser compatibility info
find . -maxdepth 3 -name 'package.json' 2>/dev/nullRepository: netalertx/NetAlertX
Length of output: 45
🌐 Web query:
ES2018 regex dotAll flag s browser support compatibility IE11 ES6
💡 Result:
The s (dotAll) flag for regular expressions was introduced as part of the ECMAScript 2018 (ES2018) specification [1][2]. It is not part of ES6 (ECMAScript 2015) [3]. The s flag allows the dot (.) character in regular expressions to match line terminator characters, which it does not do by default [4][2]. Regarding browser compatibility: - Modern browsers have supported the s flag for several years [4]. - Internet Explorer 11 (IE11) does not support the s (dotAll) flag [4]. Because IE11 is a legacy browser that does not receive updates for modern ECMAScript features, it will never natively support this flag. If you need to support environments like IE11, you must use a transpiler (such as Babel) or a polyfill (such as core-js) to provide equivalent functionality [4][5]. A common workaround in environments without s flag support is to use character classes like [\s\S] or [^] instead of the dot (.) to match any character, including newlines [2][6].
Citations:
- 1: https://github.com/tc39/proposal-regexp-dotall-flag
- 2: https://2ality.com/2017/07/regexp-dotall-flag.html
- 3: https://ponyfoo.com/articles/regular-expressions-post-es6
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/dotAll
- 5: https://core-js.io/docs/features/proposals/regexp-dotall-flag
- 6: https://dev.to/gaspricecheck/why-your-regex-is-silently-breaking-on-react-ssr-output-2h
Avoid s regex flag in vendored code for broader browser compatibility.
Line 2297 uses the dotAll flag (/…/s), an ES2018 feature that causes parse-time SyntaxError in older engines like IE11. Since moment.js is a vendored library, this will break the entire script in non-ES2018 environments. Replace with a standard workaround using [\s\S] instead:
Recommended fix
- return s.replace(/\((?:(?!\().)*\)|[\n\t]/gs, ' ').replace(/(\s\s+)/g, ' ').replace(/^\s\s*/, '').replace(/\s\s*$/, '');
+ return s.replace(/\((?:(?!\()[\s\S])*\)|[\n\t]/g, ' ').replace(/(\s\s+)/g, ' ').replace(/^\s\s*/, '').replace(/\s\s*$/, '');📝 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.
| return s.replace(/\((?:(?!\().)*\)|[\n\t]/gs, ' ').replace(/(\s\s+)/g, ' ').replace(/^\s\s*/, '').replace(/\s\s*$/, ''); | |
| return s.replace(/\((?:(?!\()[\s\S])*\)|[\n\t]/g, ' ').replace(/(\s\s+)/g, ' ').replace(/^\s\s*/, '').replace(/\s\s*$/, ''); |
🤖 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/lib/moment/moment.js` at line 2297, The vendored code uses the dotAll
`s` flag in the regex on the `s.replace(...)` line which breaks older engines;
update the pattern to avoid `s` by replacing `.` with `[\s\S]` and remove the
`s` flag (e.g. change the subpattern `(?:(?!\().)*` to `(?:(?!\()[\s\S])*`) so
the full call to `s.replace(...)` uses only `g` and no `s` flag while keeping
the other replacements (`.replace(/(\s\s+)/g, ' ')`, `.replace(/^\s\s*/, '')`,
`.replace(/\s\s*$/, '')`) unchanged.
There was a problem hiding this comment.
if i say only latest versions of browsers are supported?
There was a problem hiding this comment.
@jokob-sk, that's completely fair — if only the latest browser versions are supported, the s (dotAll) flag is perfectly fine. All modern browsers (Chrome, Firefox, Edge, Safari) have supported ES2018 regex features including dotAll for several years now. The concern only applies to legacy environments like IE11, so this is a non-issue for your setup.
✏️ Learnings added
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1634
File: front/lib/moment/moment.js:2297-2297
Timestamp: 2026-05-18T02:21:42.621Z
Learning: The NetAlertX project (netalertx/NetAlertX) targets only the latest versions of modern browsers. Legacy browser compatibility (e.g., IE11, ES5/ES6-only environments) is not a concern. ES2018+ features such as the regex `s` (dotAll) flag are acceptable in vendored and custom JS code.
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.
🧹 Nitpick comments (1)
front/lib/moment/moment.js (1)
1845-1848: ⚡ Quick winPrefer RegExp literal and
.test()for cleaner boolean check.The current implementation works, but using a RegExp literal with
.test()is more idiomatic and efficient for boolean checks.♻️ Refactor to use RegExp literal
function isLocaleNameSane(name) { // Prevent names that look like filesystem paths, i.e contain '/' or '\' - return name.match('^[^/\\\\]*$') != null; + return /^[^/\\]*$/.test(name); }🤖 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/lib/moment/moment.js` around lines 1845 - 1848, The isLocaleNameSane function uses String.prototype.match with a string pattern; replace that with a RegExp literal and .test() for a clearer boolean check (i.e. use /^[^\/\\]*$/.test(name)) so the function returns a direct boolean; locate the isLocaleNameSane function and swap the match call to the RegExp literal .test() call.
🤖 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.
Nitpick comments:
In `@front/lib/moment/moment.js`:
- Around line 1845-1848: The isLocaleNameSane function uses
String.prototype.match with a string pattern; replace that with a RegExp literal
and .test() for a clearer boolean check (i.e. use /^[^\/\\]*$/.test(name)) so
the function returns a direct boolean; locate the isLocaleNameSane function and
swap the match call to the RegExp literal .test() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d4ee395-57f9-44f5-9b50-8880ae70d593
📒 Files selected for processing (1)
front/lib/moment/moment.js
2 participants
Summary by CodeRabbit
Improvements
Documentation