#2632 add to the alarm list component the option to write comments#3194
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughImplements a revamped comment dialog lifecycle (initialization, visibility, wrapper/underlay normalization, show/hide) and integrates inline comment UI into alarm list entries with a comment icon trigger and per-event comment rendering. UI markup and CSS adjusted accordingly. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AlarmList as Alarm List UI
participant Dialog as Comment Dialog (client-side)
participant Server as Comment API
User ->> AlarmList: clicks comment icon
AlarmList ->> Dialog: call openCommentDialog(typeId, referenceId)
Dialog ->> Dialog: ensure dialog exists, unhide source, normalize wrapper/underlay, show and autofocus
User ->> Dialog: enters text and clicks save
Dialog ->> Server: saveComment(payload)
Server -->> Dialog: saveCommentCB(comment)
Dialog ->> AlarmList: insert new comment UI (template)
Dialog ->> Dialog: closeCommentDialog(), reset underlay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In @WebContent/WEB-INF/jsp/include/userComment.jsp:
- Around line 70-79: normalizeUnderlay() forces #dialogUnderlay to display:block
but closeCommentDialog() doesn't reset it, so the underlay can stay visible;
update closeCommentDialog() to explicitly find the element with id
"dialogUnderlay" and reset its styles (at minimum set display to "none" and
clear zIndex/position if applicable) after calling dlg.hide() (and also if dlg
or dlg.hide() is not present), and apply the same fix to the other close path
referenced around lines 127-130 to ensure the underlay is always hidden when the
dialog is closed.
🧹 Nitpick comments (4)
WebContent/WEB-INF/jsp/views.jsp (1)
46-55: CSS overrides are very broad; consider scoping to the comment dialog.
.dojoDialog/.commonDialog+!importantwill affect other dialogs/inputs globally, andvar(--slts-color-primary)needs a defined fallback. If the goal is just the comment dialog, scope to#CommentDialog(or a dedicated class) and add a fallback color.Proposed CSS scoping
- .dojoDialog textarea, - .dojoDialog input, - .dojoDialog select, - .commonDialog textarea, - .commonDialog input, - .commonDialog select { - border: 1px solid var(--slts-color-primary) !important; + #CommentDialog textarea, + #CommentDialog input, + #CommentDialog select { + border: 1px solid var(--slts-color-primary, #2a6ebb) !important; background: #fff !important; box-shadow: none !important; }WebContent/WEB-INF/jsp/include/userComment.jsp (3)
102-105: Add client-side empty-comment validation to avoid a DWR call.Right now you always call
MiscDwr.addUserComment(...)even for empty/whitespace input and rely onsaveCommentCB(null)to alert.Possible change
function saveComment() { - var comment = (document.getElementById("commentText") || {}).value || ""; + var comment = (document.getElementById("commentText") || {}).value || ""; + if (!comment.trim()) { + alert("<spring:message code="notes.enterComment"/>"); + return; + } MiscDwr.addUserComment(commentTypeId, commentReferenceId, comment, saveCommentCB); }
147-151: Good: dialog starts truly hidden; consider restoringaria-hidden/hiddenon close for a11y.You remove
hidden/ setaria-hidden=falseon open, but don’t restore them on close. If Dojo’shide()doesn’t restore these attributes, the dialog may remain in the accessibility tree/tab order.
34-40: Consider refactoring the defensive fallback to fail-fast or log errors rather than silently swallowing them.The
dojo.widget.parse()call on line 37 is a defensive fallback that only executes if the Dialog widget fails to initialize—which should rarely happen in normal operation sincedojo.require("dojo.widget.Dialog")at line 29 and thedojoType="dialog"markup at line 147 should trigger auto-parsing on page load. However, if initial parsing does fail, callingparse()again without scoping will re-parse the entire page, which is unnecessarily expensive. More importantly, the silent exception handler (catch(e) {}) masks underlying problems that should be logged or surfaced.Better approaches: add logging to identify why the Dialog isn't available, or use a fail-fast pattern that throws an error immediately instead of attempting recovery via full-page re-parse.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
WebContent/WEB-INF/jsp/include/userComment.jspWebContent/WEB-INF/jsp/views.jspWebContent/WEB-INF/snippet/alarmList.jsp
🔇 Additional comments (1)
WebContent/WEB-INF/jsp/include/userComment.jsp (1)
81-100: Remove or fixnormalizeUnderlay()call—the selector is incorrect.The function searches by ID (
document.getElementById("dialogUnderlay")) but Dojo's Dialog widget only sets a class name (className = "dialogUnderlay"). The element will never be found. Moving the call to aftershow()(as suggested) won't fix this. Either update the selector todocument.querySelector(".dialogUnderlay"), or remove the call if the Dojo widget's built-in styling is sufficient.Likely an incorrect or invalid review comment.
| <td> | ||
| <table cellspacing="0" cellpadding="0" width="100%"> | ||
| <colgroup> | ||
| <col /> | ||
| <col /> | ||
| <col style="width:1%" /> | ||
| </colgroup> | ||
|
|
||
| <tr> | ||
| <td colspan="2"><b><sst:i18n message="${event.message}"/></b></td> | ||
| <td align="right"> | ||
| <tag:img png="comment_add" title="notes.addNote" | ||
| onclick="openCommentDialog(${applicationScope['constants.UserComment.TYPE_EVENT']}, ${event.id})"/> | ||
| </td> | ||
| </tr> | ||
|
|
||
| <tbody id="eventComments${event.id}"> | ||
| <tag:comments comments="${event.eventComments}"/> | ||
| </tbody> | ||
| </table> | ||
| </td> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the constant is defined somewhere predictable
rg -n --hidden -S "constants\.UserComment\.TYPE_EVENT|TYPE_EVENT" | head -30Repository: SCADA-LTS/Scada-LTS
Length of output: 3626
🏁 Script executed:
#!/bin/bash
# Inspect the tag implementation/usage to see what it renders (TR vs TBODY)
rg -n --hidden -S "<tag:comments\b" -A 2 -B 2 | head -50Repository: SCADA-LTS/Scada-LTS
Length of output: 1265
🏁 Script executed:
#!/bin/bash
# Check for JSP constant patterns in other files
rg -n --hidden -S "jsp:useBean.*constants" | head -20Repository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check eventList.jsp to see if it uses a better pattern for the constant
cat WebContent/WEB-INF/snippet/eventList.jsp | head -100Repository: SCADA-LTS/Scada-LTS
Length of output: 4710
🏁 Script executed:
#!/bin/bash
# Find the tag:comments tag file implementation
fd -t f "comments.tag" WebContent/Repository: SCADA-LTS/Scada-LTS
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Inspect the comments.tag implementation
cat WebContent/WEB-INF/tags/comments.tagRepository: SCADA-LTS/Scada-LTS
Length of output: 1612
Make the "add comment" control keyboard accessible.
The tag:img element with onclick is mouse-only. Replace it with a proper <button type="button"> to ensure keyboard navigation and screen reader support. You can wrap the image inside the button for visual consistency:
<button type="button"
onclick="openCommentDialog(${applicationScope['constants.UserComment.TYPE_EVENT']}, ${event.id})"
title="<spring:message code='notes.addNote'/>">
<tag:img png="comment_add" alt=""/>
</button>The constant ${applicationScope['constants.UserComment.TYPE_EVENT']} is reliably set at startup (MangoContextListener.java:442-443) and does not risk being empty. The current table structure—with the header <tr> before the <tbody> and comment rows inside—is semantically correct per HTML specification.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @WebContent/WEB-INF/jsp/include/userComment.jsp:
- Around line 42-51: In unhideCommentSource(), replace the use of the reserved
style property name "float" with the standard DOM property cssFloat: change the
assignment that sets n.style.float = "none" to assign n.style.cssFloat = "none"
(or use style.setProperty("float", "none") if you prefer), so the function
manipulates the element's float correctly and avoids using the reserved
identifier.
🧹 Nitpick comments (1)
WebContent/WEB-INF/jsp/include/userComment.jsp (1)
34-40: Silent error suppression may hide widget initialization failures.The empty
catch(e) {}block swallows all errors during widget parsing. If widget initialization fails due to configuration or DOM issues, debugging will be difficult.Consider logging errors for diagnostics:
🔧 Suggested improvement
function ensureCommentDialog() { var dlg = dojo.widget.byId && dojo.widget.byId("CommentDialog"); if (!dlg) { - try { dojo.widget.parse(); dlg = dojo.widget.byId && dojo.widget.byId("CommentDialog"); } catch(e) {} + try { + dojo.widget.parse(); + dlg = dojo.widget.byId && dojo.widget.byId("CommentDialog"); + } catch(e) { + console.warn("CommentDialog widget parsing failed:", e); + } } return dlg; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
WebContent/WEB-INF/jsp/include/userComment.jsp
🔇 Additional comments (5)
WebContent/WEB-INF/jsp/include/userComment.jsp (5)
54-68: LGTM!The wrapper normalization logic correctly handles both the dialog's domNode and the source node fallback. The centering approach using
transform: translate(-50%, -50%)is a reliable technique.
70-93: LGTM!The underlay normalization and reset functions correctly manage the modal backdrop state. The z-index hierarchy (20000 for underlay, 20010 for wrapper) ensures proper stacking.
95-114: LGTM!The refactored dialog opening logic properly sequences initialization, visibility, and normalization. The
setTimeout(..., 0)pattern correctly ensures the dialog is fully rendered before re-normalizing and focusing.
116-119: LGTM!The safe fallback pattern
(element || {}).value || ""handles edge cases where the textarea might not exist, preventing null reference errors.
141-145: LGTM!Good accessibility practice using
aria-hiddenand the HTML5hiddenattribute for initial state. The close function properly resets the underlay state.Also applies to: 162-165
| function unhideCommentSource() { | ||
| var n = document.getElementById("CommentDialog"); | ||
| if (!n) return n; | ||
| n.removeAttribute("hidden"); | ||
| n.setAttribute("aria-hidden", "false"); | ||
| n.style.visibility = "visible"; | ||
| n.style.display = ""; | ||
| n.style.float = "none"; | ||
| return n; | ||
| } |
There was a problem hiding this comment.
Use cssFloat instead of float for CSS property access.
float is a reserved word in JavaScript. While browsers generally tolerate style.float, the standard DOM property name is cssFloat.
🔧 Suggested fix
function unhideCommentSource() {
var n = document.getElementById("CommentDialog");
if (!n) return n;
n.removeAttribute("hidden");
n.setAttribute("aria-hidden", "false");
n.style.visibility = "visible";
n.style.display = "";
- n.style.float = "none";
+ n.style.cssFloat = "none";
return n;
}📝 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.
| function unhideCommentSource() { | |
| var n = document.getElementById("CommentDialog"); | |
| if (!n) return n; | |
| n.removeAttribute("hidden"); | |
| n.setAttribute("aria-hidden", "false"); | |
| n.style.visibility = "visible"; | |
| n.style.display = ""; | |
| n.style.float = "none"; | |
| return n; | |
| } | |
| function unhideCommentSource() { | |
| var n = document.getElementById("CommentDialog"); | |
| if (!n) return n; | |
| n.removeAttribute("hidden"); | |
| n.setAttribute("aria-hidden", "false"); | |
| n.style.visibility = "visible"; | |
| n.style.display = ""; | |
| n.style.cssFloat = "none"; | |
| return n; | |
| } |
🤖 Prompt for AI Agents
In @WebContent/WEB-INF/jsp/include/userComment.jsp around lines 42 - 51, In
unhideCommentSource(), replace the use of the reserved style property name
"float" with the standard DOM property cssFloat: change the assignment that sets
n.style.float = "none" to assign n.style.cssFloat = "none" (or use
style.setProperty("float", "none") if you prefer), so the function manipulates
the element's float correctly and avoids using the reserved identifier.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.