0189-WebUI-Fix-ControlHmIP-MOD-RC8 #2892
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI
participant ControlLogic
User->>WebUI: Request control interface
WebUI->>ControlLogic: Check channelMode
alt channelMode == 3
ControlLogic-->>WebUI: Call CreateDoorWindowContact()
else channelMode != 3
ControlLogic-->>WebUI: Call CreateContent()
end
WebUI-->>User: Display updated control interface
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
buildroot-external/patches/occu/0189-WebUI-Fix-ControlHmIP-MOD-RC8/occu/WebUI/www/rega/esp/datapointconfigurator.fn (3)
682-694: Simplify nested conditions for better readabilityThe nested if-else statements can be refactored for clarity and maintainability. Simplifying the logic makes it easier to understand and reduces the potential for errors.
Refactored code:
if ((ch.HssType() == "MULTI_MODE_INPUT_TRANSMITTER") && (ch.Label() != "ELV-SH-BM-S")) { if (ch.Label().Find("MOD-RC8") != -1) { WriteLine("<script>conInfo('HmIP-MOD-RC8 found');</script>"); Call("/esp/controls/key_switch_sc.fn::CreateContent()"); } else { Call("/esp/datapointconfigurator.fn::CreateNoFunction()"); } } else if (ch.Label() == "ELV-SH-BM-S") { WriteLine("<script>conInfo('ELV-SH-BM-S found');</script>"); Call("/esp/controls/key_switch_sc.fn::CreateContentA()"); } else { Call("/esp/datapointconfigurator.fn::CreateNoFunction()"); }
91-169: Consider moving control name constants to a separate configuration fileThe numerous control name constants defined within the function clutter the code and make maintenance harder. Moving these constants to a separate configuration file or module can enhance readability and centralize the constants for easier updates.
202-203: Remove unnecessary debugging statementsThe
WriteLinestatements used for debugging purposes should be removed or commented out in the production code to avoid cluttering the console output and potentially exposing internal logic.Apply this diff to remove the debugging statements:
-WriteLine( "<script>console.log('oE.ID(): "#oE.ID()#"');</script>" );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
buildroot-external/patches/occu/0189-WebUI-Fix-ControlHmIP-MOD-RC8/occu/WebUI/www/rega/esp/datapointconfigurator.fn.origis excluded by!**/*.orig
📒 Files selected for processing (2)
buildroot-external/patches/occu/0189-WebUI-Fix-ControlHmIP-MOD-RC8.patch(1 hunks)buildroot-external/patches/occu/0189-WebUI-Fix-ControlHmIP-MOD-RC8/occu/WebUI/www/rega/esp/datapointconfigurator.fn(1 hunks)
🔇 Additional comments (3)
buildroot-external/patches/occu/0189-WebUI-Fix-ControlHmIP-MOD-RC8.patch (1)
4-9: LGTM! The change aligns with the PR objectives.
The modification correctly addresses the channel type change issue by:
- Preserving the special handling for channelMode == 3
- Using key_switch_sc.fn::CreateContent() to properly evaluate CHANNEL_OPERATION_MODE for other modes
Let's verify the key_switch_sc.fn implementation:
✅ Verification successful
The change is appropriate and the implementation is verified
The key_switch_sc.fn implementation shows that:
CreateContent()function properly handles the UI state by creating appropriate controls and initializing theiseMOD_RC8JavaScript object with the channel ID and address- The function works in conjunction with
CreateDoorWindowContact()which provides the necessary UI elements for displaying door/window states - The change from
CreateNoFunction()toCreateContent()in the patch is correct as it enables proper channel operation mode handling through theiseMOD_RC8JavaScript object
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the key_switch_sc.fn implementation and its CreateContent function
# Test 1: Check if key_switch_sc.fn exists and contains CreateContent
rg -A 5 "function\s+::CreateContent" $(fd -t f "key_switch_sc.fn$")
# Test 2: Check for any references to CHANNEL_OPERATION_MODE in key_switch_sc.fn
rg "CHANNEL_OPERATION_MODE" $(fd -t f "key_switch_sc.fn$")
Length of output: 712
Script:
#!/bin/bash
# Let's examine the content of key_switch_sc.fn to understand its implementation
cat buildroot-external/patches/occu/0039-WebUI-Fix-Style-Glitches/occu/WebUI/www/rega/esp/controls/key_switch_sc.fn
# Also check for any other patches that might modify key_switch_sc.fn
fd -t f ".*\.patch$" | xargs rg -l "key_switch_sc.fn"
Length of output: 6412
buildroot-external/patches/occu/0189-WebUI-Fix-ControlHmIP-MOD-RC8/occu/WebUI/www/rega/esp/datapointconfigurator.fn (2)
683-693: 🛠️ Refactor suggestion
Improve string comparison logic for device labels
Using ch.Label().Substr(5,7) to check for "MOD-RC8" is brittle and may fail if the label format changes. Instead, consider using the Find() method to check if "MOD-RC8" is part of the label, which is more robust and maintainable.
Apply this diff to improve the string comparison logic:
-if (ch.Label().Substr(5,7) != "MOD-RC8") {
+if (ch.Label().Find("MOD-RC8") == -1) {Likely invalid or redundant comment.
682-694:
Possible logical error in control handling condition
The condition at line 682 may not correctly represent the intended logic. If you aim to handle cases where ch.HssType() is "MULTI_MODE_INPUT_TRANSMITTER" and the label is not "ELV-SH-BM-S", the condition should use an && operator instead of ||. The current condition could skip the intended devices.
Apply this diff to correct the condition:
-if ((ch.HssType() != "MULTI_MODE_INPUT_TRANSMITTER") || (ch.Label() == "ELV-SH-BM-S")) {
+if ((ch.HssType() == "MULTI_MODE_INPUT_TRANSMITTER") && (ch.Label() != "ELV-SH-BM-S")) {Likely invalid or redundant comment.
| WriteLine('<div class="CLASS02502">' # sName # '</div>'); | ||
| WriteLine('<td>'); |
There was a problem hiding this comment.
Potential XSS vulnerability due to unescaped user input
The variable sName is inserted into the HTML output without proper escaping. If sName contains malicious content, this could lead to a Cross-Site Scripting (XSS) vulnerability. To prevent this, ensure that sName is properly escaped before being written to the output.
Apply this diff to escape sName:
-WriteLine('<div class="CLASS02502">' # sName # '</div>');
+WriteLine('<div class="CLASS02502">' # web.escapeHtml(sName) # '</div>');And at line 38:
-WriteLine('<div class="CLASS02509">'#sName#'</div></td>');
+WriteLine('<div class="CLASS02509">'#web.escapeHtml(sName)#'</div></td>');Also applies to: 38-38
jens-maus
added
🐛 bug-report
Description
Mit Version 3.79.6 wurde der Kanaltyp von
KEY_TRANSCEIVERaufMULTI_MODE_TRANSMITTERgeändert.Somit wird zum Bauen der Controls in der WebUI in eine andere Sektion eingesprungen, die verlangt, dass es zum Kanal Metadaten gibt ("channelMode") mit dem Wert "3", siehe /www/rega/esp/datapointconfigurator.fn#L591
Das Kanalobjekt hat mit der Umstellung der Kanaltyps jedoch keine zusätzlichen Metadaten bekommen.
Somit ist es notwendig, die frühere Methode
key_switch_sc.fn::CreateContent()zu verwenden.Dabei wird letztendlich der
CHANNEL_OPERATION_MODEdirekt aus dem Master-Paramset ausgewertet (/www/webui/webui.js#L41867-L41879)Related Issue
#2891
Types of changes
Alternate Designs
Possible Drawbacks
Verification Process
Release Notes
Contributing checklist
Summary by CodeRabbit
New Features
Bug Fixes