Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFrontend filter options are converted to {value,label} objects and PHP rendering now supports value/label arrays; backend filter SQL adds columnLabel and adjusts devParentMAC labeling; a scan-time backfill populates missing Devices.devSyncHubNode from SYNC_node_name; tests for the backfill were added. Changes
Sequence DiagramsequenceDiagram
participant DB as Database
participant PY as Backend (Python)
participant PHP as PHP Renderer
participant JS as Frontend
DB->>PY: Execute `sql_devices_filters` → rows with `columnValue` + `columnLabel`
PY->>PHP: Return filter rows (value,label)
PHP->>JS: Serve page with options represented as `{ value, label }`
JS->>JS: Deduplicate by `value`, sort by `label` and render dropdown
sequenceDiagram
participant Scheduler as Scan Runner
participant Session as session_events.process_scan
participant Handler as device_handling.update_sync_hub_node
participant DB as Database
Scheduler->>Session: start process_scan()
Session->>DB: update_devices_data_from_scan()
Session->>Handler: call update_sync_hub_node(db)
Handler->>DB: UPDATE Devices SET devSyncHubNode = <SYNC_node_name> WHERE COALESCE(LOWER(TRIM(devSyncHubNode)), '') IN {NULL_EQUIVALENTS_SQL}
Handler->>DB: db.commitDB()
Session->>DB: update_devLastConnection_from_CurrentScan()
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
front/devices.php (1)
458-460: Consider improving numeric string sorting in filter options.The current code sorts filter labels using
localeCompare(b.label), which sorts numeric strings lexicographically (e.g., "10", "100", "20"). For VLAN IDs and similar numeric-like values, consider usinglocaleCompare(..., undefined, { numeric: true })for more intuitive sorting. Additionally, wrap the label inString()as a defensive measure:Suggested improvement
transformed.filters.forEach(filter => { - filter.options.sort((a, b) => a.label.localeCompare(b.label)); + filter.options.sort((a, b) => + String(a.label ?? '').localeCompare( + String(b.label ?? ''), + undefined, + { numeric: true, sensitivity: 'base' } + ) + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/devices.php` around lines 458 - 460, The sort currently uses filter.options.sort((a, b) => a.label.localeCompare(b.label)), which orders numeric strings lexicographically; update the comparator in the transformed.filters.forEach loop to call localeCompare with the numeric option and defend against non-string labels by wrapping labels with String(), e.g., use String(a.label).localeCompare(String(b.label), undefined, { numeric: true }) so VLAN-like numeric labels sort intuitively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/const.py`:
- Around line 99-104: The SELECT uses COALESCE(p.devName, d.devParentMAC) which
does not treat empty strings as NULL, producing blank dropdown labels; update
the expression so empty p.devName falls back to d.devParentMAC (e.g., replace
COALESCE(p.devName, d.devParentMAC) with COALESCE(NULLIF(p.devName, ''),
d.devParentMAC) or an equivalent CASE/NULLIF pattern) in the SELECT that builds
columnLabel (the query using aliases p and d and columnLabel).
---
Nitpick comments:
In `@front/devices.php`:
- Around line 458-460: The sort currently uses filter.options.sort((a, b) =>
a.label.localeCompare(b.label)), which orders numeric strings lexicographically;
update the comparator in the transformed.filters.forEach loop to call
localeCompare with the numeric option and defend against non-string labels by
wrapping labels with String(), e.g., use
String(a.label).localeCompare(String(b.label), undefined, { numeric: true }) so
VLAN-like numeric labels sort intuitively.
🪄 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: a74e0c47-7154-4abc-b9d0-3ea76dc74216
📒 Files selected for processing (3)
front/devices.phpfront/php/components/devices_filters.phpserver/const.py
…ling devSyncHubNode #1609
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/scan/device_handling.py`:
- Around line 249-259: Normalize the retrieved setting and existing DB values
before backfilling: call get_setting_value and immediately trim whitespace and
normalize case (e.g., node_name = get_setting_value(...).strip().lower()), then
return if node_name is empty; in the UPDATE statement for Devices (the
devSyncHubNode column referenced in the COALESCE(...) condition), compare
against trimmed/lowercased DB values (use SQL functions like TRIM/LOWER on
devSyncHubNode) and the normalized literals ('' and 'null') so values like "
NULL " or whitespace-only settings are treated as empty and won't be persisted
as node names.
🪄 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: 2cbf4548-00d3-4050-a2c1-bd531ff86754
📒 Files selected for processing (3)
server/scan/device_handling.pyserver/scan/session_events.pytest/scan/test_sync_hub_node_backfill.py
Signed-off-by: jokob-sk <jokob.sk@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/scan/device_handling.py`:
- Around line 243-263: In update_sync_hub_node, the SQL IN clause uses
NULL_EQUIVALENTS_SQL without surrounding parentheses causing invalid SQL; update
the f-string in the sql.execute call inside update_sync_hub_node to wrap
NULL_EQUIVALENTS_SQL in parentheses (i.e. use IN ({NULL_EQUIVALENTS_SQL})) so
the comma-separated list is a valid SQL tuple, then run db.commitDB() as before.
🪄 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: df40f25f-0a10-440d-9605-6c54ade28bd8
📒 Files selected for processing (2)
server/const.pyserver/scan/device_handling.py
Signed-off-by: jokob-sk <jokob.sk@gmail.com>
Contributor
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/scan/device_handling.py (1)
403-412:⚠️ Potential issue | 🔴 CriticalParameterize
default_iconin the SELECT filter.Line 404 interpolates a user-configurable setting directly into SQL (
'{default_icon}'). That can break query semantics and is an injection vector.🔧 Proposed fix
if get_setting_value("NEWDEV_replace_preset_icon"): query = f"""SELECT * FROM Devices - WHERE devIcon in ({NULL_EQUIVALENTS_SQL}, '{default_icon}') + WHERE devIcon in ({NULL_EQUIVALENTS_SQL}, ?) OR devIcon IS NULL""" + params = (default_icon,) else: query = f"""SELECT * FROM Devices WHERE devIcon in ({NULL_EQUIVALENTS_SQL}) OR devIcon IS NULL""" + params = () - for device in sql.execute(query): + for device in sql.execute(query, params):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/scan/device_handling.py` around lines 403 - 412, The SELECT currently injects default_icon into the SQL string (devIcon in (..., '{default_icon}')), which is an injection risk; change the code that builds the query to use a bound parameter for default_icon (e.g., a placeholder like ? or %s depending on the DB driver) and pass default_icon as an argument to sql.execute instead of interpolating it. Locate the branch that references NULL_EQUIVALENTS_SQL and default_icon in device_handling.py (the query variable and the subsequent sql.execute loop) and replace the inline '{default_icon}' with a parameter placeholder and include default_icon in the execute call so the DB driver handles escaping/typing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/scan/device_handling.py`:
- Around line 403-412: The SELECT currently injects default_icon into the SQL
string (devIcon in (..., '{default_icon}')), which is an injection risk; change
the code that builds the query to use a bound parameter for default_icon (e.g.,
a placeholder like ? or %s depending on the DB driver) and pass default_icon as
an argument to sql.execute instead of interpolating it. Locate the branch that
references NULL_EQUIVALENTS_SQL and default_icon in device_handling.py (the
query variable and the subsequent sql.execute loop) and replace the inline
'{default_icon}' with a parameter placeholder and include default_icon in the
execute call so the DB driver handles escaping/typing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3ee545d-e5dc-48d2-a971-2b786c842fa2
📒 Files selected for processing (1)
server/scan/device_handling.py
…shared DB helpers
…into next_release
Collaborator
Author
|
review @coderabbitai |
Contributor
|
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit