Conversation
Signed-off-by: jokob-sk <[email protected]>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip You can get early access to new features in CodeRabbit.Enable the WalkthroughAutomatic API token generation added when Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Settings file
participant Init as initialise.py
participant Rename as renameSettings
participant Util as generate_random_string
Config->>Init: load settings
Init->>Rename: build replacements (includes API_TOKEN mapping)
Rename->>Rename: detect API_TOKEN == ''
Rename->>Util: generate_random_string(20)
Util-->>Rename: return t_<20-char>
Rename-->>Init: apply replacement (API_TOKEN='t_<20-char>')
Init->>Config: persist updated settings
sequenceDiagram
participant User as user input
participant Nmap as nmap_dev.py
participant CLI as nmap executable
User->>Nmap: provide interface (may include --vlan=...)
Note right of Nmap: Preprocess — strip `--vlan=...`
Nmap->>Nmap: clean interface string
Nmap->>CLI: execute scan with cleaned args
CLI-->>Nmap: return results
sequenceDiagram
participant PM as plugin_manager
participant Plugin as plugin script
participant DB as pm.db / Plugins_Objects
participant App as app_state (updateState)
participant NameUp as update_devices_names(pm)
Plugin->>PM: run plugin (scheduled or ad-hoc)
PM->>DB: read/update plugin objects
PM->>PM: get_plugin_states(plugin_name)
PM->>App: updateState(pluginsStates=...)
PM->>NameUp: call update_devices_names(pm)
NameUp->>DB: read plugin state, maybe resolve names
NameUp->>DB: commit changes via pm.db.commitDB()
PM->>PM: pm.name_plugins_checked = currentTime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/initialise.py (1)
454-458: Fix the f-string timing issue: random token generated at module import, not per replacement.The
renameSettingsfunction executes (called fromserver/__main__.pyline 100) and will process the replacements. However, the f-string at line 457 has a critical security flaw:
generate_random_string(20)evaluates at module import time (whenserver/initialise.pyis imported), not when replacements occur. This means:
- All empty
API_TOKENvalues in a config file receive the same random token- The token is reused across all config migrations
- This defeats the purpose of random token generation
Solution: Generate a unique token for each replacement:
replacements = { r'\bREPORT_TO\b': 'SMTP_REPORT_TO', r'\bSYNC_api_token\b': 'API_TOKEN', r'\bAPI_TOKEN=\'\'': None, # Will be handled specially }Then in the
renameSettingsfunction (around line 492), modify the replacement loop:for key, value in replacements.items(): if key == r'\bAPI_TOKEN=\'\'': value = f'API_TOKEN=\'t_{generate_random_string(20)}\'' line = re.sub(key, value, line)This generates a fresh token for each replacement during execution, not at import time.
🧹 Nitpick comments (1)
server/initialise.py (1)
452-452: Consider using standard terminology "DEPRECATED".The comment header uses "DEPRECATE" which, while better than the previous typo, differs from the standard term "DEPRECATED" commonly used in code deprecation notices.
Apply this diff if you'd like to use standard terminology:
-# DEPRECATE soonest after 10/10/2024 +# DEPRECATED soonest after 10/10/2024
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/initialise.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/initialise.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Add or modify settings centrally via 'ccd()' in 'server/initialise.py' or via plugin manifest; never hardcode ports or secrets—always use 'get_setting_value()'.
Files:
server/initialise.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
server/initialise.py
⏰ 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
Signed-off-by: jokob-sk <[email protected]>
Signed-off-by: jokob-sk <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/app_state.py (1)
140-140: Misleading comment: return statement doesn't enable chaining.The comment states "Allows chaining by returning self" but the bare
returnstatement returnsNone, which doesn't enable method chaining.Either remove the misleading comment:
- return # Allows chaining by returning self + returnOr return
selfif chaining is desired:- return # Allows chaining by returning self + return self # Allows chaining by returning self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/__main__.py(1 hunks)server/app_state.py(7 hunks)server/db/db_upgrade.py(1 hunks)server/plugin.py(4 hunks)server/scan/device_handling.py(3 hunks)server/scan/name_resolution.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/db/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer using functions from 'server/db/db_helper.py' (like 'get_table_json' and device condition helpers) over raw SQL for new database code.
Files:
server/db/db_upgrade.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
server/db/db_upgrade.pyserver/scan/device_handling.pyserver/__main__.pyserver/scan/name_resolution.pyserver/plugin.pyserver/app_state.py
🧠 Learnings (1)
📚 Learning: 2025-10-21T01:40:24.597Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-21T01:40:24.597Z
Learning: Applies to front/plugins/*/script.py : Run plugin scripts as standalone executables with 'sys.path' configured for '/app/front/plugins' and '/app/server', following the provided template.
Applied to files:
server/plugin.py
🧬 Code graph analysis (4)
server/scan/device_handling.py (5)
server/scan/name_resolution.py (5)
NameResolver(24-83)resolve_dig(65-66)resolve_mdns(56-57)resolve_nslookup(59-60)resolve_nbtlookup(62-63)server/models/device_instance.py (1)
DeviceInstance(12-84)server/helper.py (1)
timeNowTZ(35-45)server/logger.py (2)
mylog(86-92)timeNowTZ(16-20)server/database.py (1)
commitDB(79-91)
server/__main__.py (2)
server/plugin.py (1)
run_plugin_scripts(56-103)server/scan/device_handling.py (1)
update_devices_names(519-642)
server/plugin.py (1)
server/app_state.py (1)
updateState(146-174)
server/app_state.py (1)
server/helper.py (1)
default(752-756)
⏰ 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 (5)
server/app_state.py (5)
19-33: LGTM! Comprehensive documentation added.The class-level docstring clearly documents all attributes including the new
pluginsStatesfeature.
35-56: LGTM! Well-documented parameter addition.The
pluginsStatesparameter and its merging behavior are clearly documented in the docstring.
81-81: LGTM! Safe initialization with defaults.The
pluginsStatesis properly initialized to an empty dict when not present in previous state.Also applies to: 91-91
146-174: LGTM! Function signature updated consistently.The
updateStatefunction properly accepts and propagates thepluginsStatesparameter with clear documentation.
180-189: LGTM! Clear encoder documentation.The
AppStateEncoderdocstring clearly explains its purpose for serializing objects with__dict__attributes.
server/scan/device_handling.py
Outdated
| latest_change = max( | ||
| [pm.plugin_states.get(p, {}).get("lastChanged") for p in name_plugins if pm.plugin_states.get(p)], | ||
| default=None | ||
| ) | ||
|
|
||
| # Convert to comparable datetime if needed | ||
| from dateutil import parser | ||
| latest_change_dt = parser.parse(latest_change) if latest_change else None | ||
|
|
||
| # Skip if nothing changed since last check | ||
| if last_checked and latest_change_dt and latest_change_dt <= last_checked: | ||
| mylog('debug', '[Update Device Name] No relevant plugin changes since last check, skipping.') | ||
| return |
There was a problem hiding this comment.
Prevent runtime errors when comparing plugin timestamps
When a name-resolving plugin has no objects yet, lastChanged comes back as an empty string. parser.parse("") raises ValueError, so the very first call to update_devices_names(pm) will crash. Once the plugins do produce data, the comparison latest_change_dt <= last_checked now mixes a naive timestamp from the DB with the timezone-aware value returned by timeNowTZ(), which raises TypeError. Either way we currently blow up the whole name-resolution pass. Please filter out falsy timestamps and normalize both datetimes before comparing.
- latest_change = max(
- [pm.plugin_states.get(p, {}).get("lastChanged") for p in name_plugins if pm.plugin_states.get(p)],
- default=None
- )
-
- # Convert to comparable datetime if needed
- from dateutil import parser
- latest_change_dt = parser.parse(latest_change) if latest_change else None
-
- # Skip if nothing changed since last check
- if last_checked and latest_change_dt and latest_change_dt <= last_checked:
+ timestamps = [
+ ts for ts in (
+ pm.plugin_states.get(p, {}).get("lastChanged")
+ for p in name_plugins
+ )
+ if ts
+ ]
+
+ from dateutil import parser
+ latest_change_dt = parser.parse(max(timestamps)) if timestamps else None
+
+ last_checked_cmp = last_checked
+ if last_checked_cmp and last_checked_cmp.tzinfo is not None:
+ last_checked_cmp = last_checked_cmp.replace(tzinfo=None)
+ latest_change_cmp = latest_change_dt
+ if latest_change_cmp and latest_change_cmp.tzinfo is not None:
+ latest_change_cmp = latest_change_cmp.replace(tzinfo=None)
+
+ # Skip if nothing changed since last check
+ if last_checked_cmp and latest_change_cmp and latest_change_cmp <= last_checked_cmp:
mylog('debug', '[Update Device Name] No relevant plugin changes since last check, skipping.')
returnThere was a problem hiding this comment.
changed, review again
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current implementation of update_devices_names function
ast-grep --pattern $'def update_devices_names($$$) {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get the update_devices_names function from the current file
sed -n '/^def update_devices_names/,/^def /p' server/scan/device_handling.py | head -n -1
Length of output: 6020
🏁 Script executed:
#!/bin/bash
# Check how stateUpdated is set in plugin_states (get_plugin_states method)
rg -n -A 30 "def get_plugin_states" server/plugin.py | head -60
Length of output: 1376
🏁 Script executed:
#!/bin/bash
# Check timeNowTZ implementation and timezone handling
rg -n -B2 -A10 "def timeNowTZ" server/helper.py
Length of output: 529
@jokob-sk, I can see the changes you made, but there are still critical runtime bugs that will crash name resolution:
Remaining Issues
1. Empty string handling in stateUpdated
When a plugin has no objects yet, get_plugin_states() returns stateUpdated = "" (line 232 in plugin.py). This flows through:
state_times = [
pm.plugin_states.get(p, {}).get("stateUpdated") # Can be ""
for p in name_plugins
if pm.plugin_states.get(p) # Only checks key exists, not value
]
latest_state_str = max(state_times, default=None) # Could be ""
latest_state_dt = parser.parse(latest_state_str) if latest_state_str else None # "" is truthy!Problem: "" is truthy in Python, so parser.parse("") will be called and raise ParserError.
2. Timezone mismatch in comparison
CURRENT_TIMESTAMP(SQLite) returns naive datetime stringstimeNowTZ()returns timezone-aware datetime whenconf.tzis set- If
pm.name_plugins_checkedwas set elsewhere withtimeNowTZ(), it's timezone-aware - Comparing aware vs naive datetimes raises
TypeError
Fix Required
# Filter out empty/None values from state_times
state_times = [
pm.plugin_states.get(p, {}).get("stateUpdated")
for p in name_plugins
if pm.plugin_states.get(p, {}).get("stateUpdated") # Check value exists and is truthy
]
latest_state_str = max(state_times) if state_times else None
latest_state_dt = parser.parse(latest_state_str) if latest_state_str else None
# Normalize both datetimes to naive for comparison
last_checked_cmp = last_checked_dt.replace(tzinfo=None) if last_checked_dt and last_checked_dt.tzinfo else last_checked_dt
latest_change_cmp = latest_state_dt.replace(tzinfo=None) if latest_state_dt and latest_state_dt.tzinfo else latest_state_dt
# Skip if no plugin state changed since last check
if last_checked_cmp and latest_change_cmp and latest_change_cmp <= last_checked_cmp:
mylog('debug', '[Update Device Name] No relevant name plugin changes since last check — skipping update.')
return🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
There was a problem hiding this comment.
This is how name_plugins_checked is set:
# --- Step 3: Log last checked time ---
# After resolving names, update last checked
sql = pm.db.sql
sql.execute("SELECT CURRENT_TIMESTAMP")
row = sql.fetchone()
pm.name_plugins_checked = row[0] if row else None
There was a problem hiding this comment.
@jokob-sk, thank you for the clarification! That resolves the timezone concern I raised. Since pm.name_plugins_checked is always set from SQLite's CURRENT_TIMESTAMP (which returns a naive datetime string), and stateUpdated also comes from database queries, both timestamps are naive when parsed, so the comparison will work correctly.
Current Implementation Status
Your implementation is actually correct as-is:
-
Empty string handling ✅
When a plugin has no objects,stateUpdated = "", which gets included instate_times. However,if latest_state_strcorrectly treats""as falsy, soparser.parse("")is never called. If all plugins have empty timestamps,latest_state_dt = None, and the short-circuit condition fails, allowing name resolution to proceed (correct behavior for first run). -
Timezone handling ✅
Both timestamps are naive strings from SQLite, parsed to naive datetimes, so the comparison works without raisingTypeError. -
None handling ✅
InitialNonevalue forpm.name_plugins_checkedis handled by the guardif last_checked_dt and latest_state_dt.
Optional: Minor Clarity Improvement
For explicit intent, you could filter empty strings upfront, though the current code handles them correctly:
state_times = [
pm.plugin_states.get(p, {}).get("stateUpdated")
for p in name_plugins
if pm.plugin_states.get(p, {}).get("stateUpdated") # Explicitly check value is truthy
]
latest_state_str = max(state_times) if state_times else NoneBut this is optional—the current implementation is functionally sound. The code is working as intended! 🎉
Signed-off-by: jokob-sk <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/scan/device_handling.py (1)
528-547: Prevent crashes from timezone-aware comparison and malformed timestamps.Line 542's
parser.parse()will raiseValueErrororParserErroriflatest_state_strcontains an invalid timestamp format. Line 545's comparisonlatest_state_dt <= last_checked_dtwill raiseTypeErrorwhen one datetime is timezone-aware and the other is naive. Both will crash the name-resolution pass.Apply this diff to add exception handling and normalize timezones:
- # Retrieve last time name resolution was checked (string or datetime) - last_checked_str = pm.name_plugins_checked - last_checked_dt = parser.parse(last_checked_str) if isinstance(last_checked_str, str) else last_checked_str - # Find the most recent plugin update time among name-related plugins state_times = [ pm.plugin_states.get(p, {}).get("stateUpdated") for p in name_plugins if pm.plugin_states.get(p) ] latest_state_str = max(state_times, default=None) - latest_state_dt = parser.parse(latest_state_str) if latest_state_str else None - + + # Parse timestamps with exception handling + last_checked_dt = None + latest_state_dt = None + try: + last_checked_str = pm.name_plugins_checked + if isinstance(last_checked_str, str) and last_checked_str: + last_checked_dt = parser.parse(last_checked_str) + elif last_checked_str: + last_checked_dt = last_checked_str + + if latest_state_str: + latest_state_dt = parser.parse(latest_state_str) + except (ValueError, parser.ParserError) as e: + mylog('debug', f'[Update Device Name] Error parsing timestamps: {e}') + # Continue without short-circuit + last_checked_dt = None + latest_state_dt = None + + # Normalize to naive datetime for comparison + if last_checked_dt and last_checked_dt.tzinfo: + last_checked_dt = last_checked_dt.replace(tzinfo=None) + if latest_state_dt and latest_state_dt.tzinfo: + latest_state_dt = latest_state_dt.replace(tzinfo=None) + # Skip if no plugin state changed since last check if last_checked_dt and latest_state_dt and latest_state_dt <= last_checked_dt: - mylog('debug', '[Update Device Name] No relevant name plugin changes since last check — skipping update.') + mylog('debug', '[Update Device Name] No relevant name plugin changes since last check; skipping update.') return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/app_state.py(7 hunks)server/plugin.py(4 hunks)server/scan/device_handling.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
server/app_state.pyserver/scan/device_handling.pyserver/plugin.py
🧠 Learnings (2)
📚 Learning: 2025-11-02T02:22:10.949Z
Learnt from: jokob-sk
Repo: jokob-sk/NetAlertX PR: 1261
File: server/app_state.py:106-115
Timestamp: 2025-11-02T02:22:10.949Z
Learning: In server/app_state.py, the pluginsStates parameter always contains complete plugin state objects with the structure: {"PLUGIN_NAME": {"lastChanged": "...", "totalObjects": N, "newObjects": N, "changedObjects": N}}. Type validation before calling .update() is not needed as the maintainer guarantees well-formed objects are always supplied.
Applied to files:
server/app_state.pyserver/plugin.py
📚 Learning: 2025-10-21T01:40:24.597Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-21T01:40:24.597Z
Learning: Applies to front/plugins/*/script.py : Run plugin scripts as standalone executables with 'sys.path' configured for '/app/front/plugins' and '/app/server', following the provided template.
Applied to files:
server/plugin.py
🧬 Code graph analysis (3)
server/app_state.py (1)
server/helper.py (1)
default(752-756)
server/scan/device_handling.py (5)
server/helper.py (2)
get_setting_value(345-398)default(752-756)server/logger.py (2)
mylog(86-92)Logger(55-96)server/models/device_instance.py (1)
DeviceInstance(12-84)server/scan/name_resolution.py (5)
NameResolver(24-83)resolve_dig(65-66)resolve_mdns(56-57)resolve_nslookup(59-60)resolve_nbtlookup(62-63)server/database.py (1)
commitDB(79-91)
server/plugin.py (1)
server/app_state.py (1)
updateState(150-178)
⏰ 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 (13)
server/plugin.py (4)
29-30: LGTM! Instance variables correctly initialized.The initialization of
plugin_statesandname_plugins_checkedprovides the necessary state tracking for plugin management and name resolution timing.
94-96: LGTM! Plugin state correctly updated after execution.The pattern of computing the latest plugin state and pushing it to app_state after execution ensures the UI receives timely updates.
165-178: LGTM! Consistent state update pattern in ad-hoc execution.The same state-update pattern used in scheduled runs is correctly applied to ad-hoc plugin execution, maintaining consistency across execution paths.
233-271: LGTM! Plugin state computation is well-structured.The method correctly handles both single-plugin and full-refresh modes, derives metrics from the database, and maintains an in-memory cache for efficient lookups.
server/scan/device_handling.py (4)
6-6: LGTM! Dependencies correctly imported and logger initialized.The
dateutil.parserimport and early logger initialization ensure date parsing capabilities and proper logging throughout the module.Also applies to: 20-21
523-527: LGTM! Function signature updated to plugin-manager context.The signature change from
update_devices_names(db)toupdate_devices_names(pm)aligns with the broader migration to plugin-manager-based state management.
553-556: LGTM! Strategy labels updated consistently.The resolution strategy names (DIGSCAN/AVAHISCAN/NSLOOKUP/NBTSCAN) and corresponding log messages are consistently updated throughout the function.
Also applies to: 618-618, 635-635
642-649: LGTM! Last-checked timestamp correctly persisted.Recording the current timestamp after successful name resolution ensures the short-circuit logic can accurately determine when plugins have changed.
server/app_state.py (5)
19-56: LGTM! Comprehensive class documentation.The docstring clearly describes all attributes and initialization parameters, improving code maintainability.
81-81: LGTM! Plugin states correctly loaded or initialized.Loading
pluginsStatesfrom previous state ensures continuity across restarts, with a sensible fallback to an empty dict on first run.Also applies to: 91-91
106-119: LGTM! Defensive merge logic protects against corruption.The type checking before calling
.update()preventsAttributeErrorif corrupted state is encountered, even though the maintainer guarantees well-formed objects. This defensive approach is good practice.Based on learnings.
150-178: LGTM! Well-documented convenience method.The
updateStatefunction docstring clearly describes all parameters and return type, making the API easy to use correctly.
184-193: LGTM! Encoder documentation added.The
AppStateEncoderdocstring clarifies its purpose for automatic serialization of objects with__dict__attributes.
Signed-off-by: jokob-sk <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/app_state.py(7 hunks)server/plugin.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
server/app_state.pyserver/plugin.py
🧠 Learnings (2)
📚 Learning: 2025-11-02T02:22:10.949Z
Learnt from: jokob-sk
Repo: jokob-sk/NetAlertX PR: 1261
File: server/app_state.py:106-115
Timestamp: 2025-11-02T02:22:10.949Z
Learning: In server/app_state.py, the pluginsStates parameter always contains complete plugin state objects with the structure: {"PLUGIN_NAME": {"lastChanged": "...", "totalObjects": N, "newObjects": N, "changedObjects": N}}. Type validation before calling .update() is not needed as the maintainer guarantees well-formed objects are always supplied.
Applied to files:
server/app_state.pyserver/plugin.py
📚 Learning: 2025-10-21T01:40:24.597Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-21T01:40:24.597Z
Learning: Applies to front/plugins/*/script.py : Run plugin scripts as standalone executables with 'sys.path' configured for '/app/front/plugins' and '/app/server', following the provided template.
Applied to files:
server/plugin.py
🧬 Code graph analysis (2)
server/app_state.py (1)
server/helper.py (1)
default(752-756)
server/plugin.py (1)
server/app_state.py (1)
updateState(150-178)
⏰ 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
| # Update plugin states in app_state | ||
| current_plugin_state = self.get_plugin_states(prefix) # get latest plugin state | ||
| updateState(pluginsStates={prefix: current_plugin_state.get(prefix, {})}) | ||
|
|
There was a problem hiding this comment.
Don't clobber the spinner state.
Calling updateState with only pluginsStates still passes showSpinner=False (per the helper’s signature), so each call here immediately clears the spinner—even if another flow just turned it on (e.g., a user-initiated run that should keep the spinner visible until completion). Please pass showSpinner=None (or adjust updateState to default to None) so plugin state refreshes don’t break the UI feedback loop.
- updateState(pluginsStates={prefix: current_plugin_state.get(prefix, {})})
+ updateState(showSpinner=None, pluginsStates={prefix: current_plugin_state.get(prefix, {})})
…
- updateState(pluginsStates={pluginName: current_plugin_state.get(pluginName, {})})
+ updateState(showSpinner=None, pluginsStates={pluginName: current_plugin_state.get(pluginName, {})})Also applies to: 173-176
🤖 Prompt for AI Agents
In server/plugin.py around lines 94-97 (and also apply the same fix at 173-176),
calling updateState with only pluginsStates relies on the helper's default
showSpinner=False which inadvertently clears a visible spinner; change the calls
to pass showSpinner=None (i.e., updateState(pluginsStates={...},
showSpinner=None)) so the spinner state is preserved when refreshing plugin
state, or alternatively adjust updateState's signature to default showSpinner to
None and keep the call sites unchanged.
Signed-off-by: jokob-sk <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/app_state.py (1)
106-119: Remove unnecessary type validation.Lines 111-115 implement type checking before calling
.update(), but this contradicts the established guarantee thatpluginsStatesalways contains complete, well-formed plugin state objects. The defensive code adds complexity without benefit.Based on learnings.
Apply this diff to simplify the merge logic:
# Merge plugin states instead of overwriting if pluginsStates is not None: for plugin, state in pluginsStates.items(): if plugin in self.pluginsStates: - # Only update existing keys if both are dicts - if isinstance(self.pluginsStates[plugin], dict) and isinstance(state, dict): - self.pluginsStates[plugin].update(state) - else: - # Replace if types don't match - self.pluginsStates[plugin] = state + # Update existing keys + self.pluginsStates[plugin].update(state) else: # Optionally ignore or add new plugin entries # To ignore new plugins, comment out the next line self.pluginsStates[plugin] = state
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/app_state.py(7 hunks)server/scan/device_handling.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
server/app_state.pyserver/scan/device_handling.py
🧠 Learnings (1)
📚 Learning: 2025-11-02T02:22:10.949Z
Learnt from: jokob-sk
Repo: jokob-sk/NetAlertX PR: 1261
File: server/app_state.py:106-115
Timestamp: 2025-11-02T02:22:10.949Z
Learning: In server/app_state.py, the pluginsStates parameter always contains complete plugin state objects with the structure: {"PLUGIN_NAME": {"lastChanged": "...", "totalObjects": N, "newObjects": N, "changedObjects": N}}. Type validation before calling .update() is not needed as the maintainer guarantees well-formed objects are always supplied.
Applied to files:
server/app_state.pyserver/scan/device_handling.py
🧬 Code graph analysis (2)
server/app_state.py (1)
server/helper.py (1)
default(752-756)
server/scan/device_handling.py (5)
server/helper.py (2)
get_setting_value(345-398)default(752-756)server/logger.py (2)
mylog(86-92)Logger(55-96)server/models/device_instance.py (1)
DeviceInstance(12-84)server/scan/name_resolution.py (5)
NameResolver(24-83)resolve_dig(65-66)resolve_mdns(56-57)resolve_nslookup(59-60)resolve_nbtlookup(62-63)server/database.py (1)
commitDB(79-91)
⏰ 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 (9)
server/app_state.py (6)
19-33: LGTM: Excellent documentation.The comprehensive class-level docstring clearly describes all attributes with their types and purposes, improving code maintainability.
35-56: LGTM: Clear constructor documentation.The updated signature and comprehensive docstring accurately describe the initialization flow and parameter behavior.
81-81: LGTM: Proper state initialization.Plugin states are correctly initialized from previous state or defaulted to an empty dict, consistent with other state attributes.
Also applies to: 91-91
144-144: LGTM: Method chaining enabled.Returning
selffrom__init__is unconventional but valid. The comment clearly explains the intent to enable method chaining.
150-178: LGTM: Clean state update wrapper.The updated signature and comprehensive docstring accurately reflect the expanded functionality. The implementation correctly passes all parameters through to the constructor.
184-188: LGTM: Clear encoder documentation.The docstring accurately describes the JSON encoder's behavior for serializing objects with
__dict__attributes.server/scan/device_handling.py (3)
6-6: LGTM: Imports and logger initialization.The addition of
dateutil.parseris appropriate for the timestamp parsing needed in the refactored name resolution logic. The module-level Logger initialization ensures logging is available throughout the module, which is fine as long as settings are loaded before this module is imported.Also applies to: 13-13, 20-21
523-549: Well-implemented short-circuit logic with robust filtering.The refactored signature and plugin-state-aware early return are working correctly. The timestamp comparison concerns from the previous review have been properly resolved—both
pm.name_plugins_checkedandstateUpdatedvalues come from SQLite'sCURRENT_TIMESTAMP, so both are naive datetimes and can be safely compared.The filtering at line 539 (
if state_updated and state_updated.strip()) is actually more defensive than what was suggested in the previous review, correctly excluding None, empty strings, and whitespace-only strings. This prevents any possibility of passing invalid input toparser.parse().Based on previous review discussion and maintainer clarification.
555-559: LGTM: Strategy names updated consistently and timestamp persistence implemented correctly.The resolution strategies have been properly renamed to match the new plugin names (DIGSCAN, AVAHISCAN, NSLOOKUP, NBTSCAN), with corresponding updates in both the strategy definitions and log messages. The database operations correctly use
pm.dbthroughout, and the timestamp persistence logic (lines 646-651) properly updatespm.name_plugins_checkedusingCURRENT_TIMESTAMP, ensuring consistent naive datetime handling for future comparisons.Also applies to: 620-620, 637-637, 644-651
Signed-off-by: jokob-sk <[email protected]>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores