-
Notifications
You must be signed in to change notification settings - Fork 5
feat: switch from angular/AJAX+Boostrap to HTMX+PicoCSS #345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| <aside> | ||
| <nav> | ||
| <ul> | ||
| <c:forEach var="requirement" items="${resources}"> | ||
| <li> | ||
| <a href="${requirement.about}" | ||
| onclick="sendOslcSelectionPostMessage(this, event)" | ||
| >${requirement.title}</a> | ||
| </li> | ||
| </c:forEach> | ||
| </ul> | ||
| </nav> | ||
| </aside> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tiny bit of HTML is what gets rendered every time HTMX makes a search request and it dynamically replaces the contents of the #search-results div with this content returned by the server. Simple and easy.
7f598be to
b3d8e02
Compare
513467a to
d3a4031
Compare
b25d924 to
4cb0c01
Compare
4cb0c01 to
8872d81
Compare
ab8d340 to
b8d6411
Compare
72e2710 to
707ebc9
Compare
707ebc9 to
1b599a3
Compare
254d884 to
fc40afc
Compare
024f893 to
74ba99c
Compare
66bcc5a to
b610c33
Compare
b91907a to
d1c1216
Compare
3485858 to
44a1d94
Compare
44a1d94 to
2346d58
Compare
2346d58 to
f1e12c1
Compare
f1e12c1 to
8b29ca2
Compare
59b167c to
d1bb9b2
Compare
d90a54d to
597ab05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (2)
src/server-rm/src/main/webapp/co/oslc/refimpl/rm/gen/requirementselector-results.jsp:29
- The requirement resource title is rendered directly into the HTML without escaping. If a requirement title contains HTML special characters or malicious script tags, this could lead to XSS vulnerabilities. Use JSTL's c:out tag with escapeXml="true" or fn:escapeXml() to properly escape the title content.
>${requirement.title}</a>
src/server-rm/src/main/webapp/co/oslc/refimpl/rm/gen/requirementselector-results.jsp:27
- The requirement.about URI is rendered directly in the href attribute without escaping. If the URI contains quotes or other special characters, it could break the attribute or lead to attribute injection attacks. Use JSTL c:out or fn:escapeXml() to properly escape the URI.
<a href="${requirement.about}"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| ] | ||
| } | ||
| window.parent.postMessage("oslc-response:" + JSON.stringify(message), '*') |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wildcard origin '*' in postMessage allows any website to receive the OSLC selection response, which could expose sensitive resource information to malicious sites. Consider using the opener's origin or a configured whitelist of allowed origins instead.
| window.parent.postMessage("oslc-response:" + JSON.stringify(message), '*') | |
| // Extract parent origin from document.referrer | |
| let parentOrigin = null; | |
| try { | |
| if (document.referrer) { | |
| parentOrigin = new URL(document.referrer).origin; | |
| } | |
| } catch (e) { | |
| // If referrer is malformed, do not send the message | |
| parentOrigin = null; | |
| } | |
| if (parentOrigin) { | |
| window.parent.postMessage("oslc-response:" + JSON.stringify(message), parentOrigin); | |
| } else { | |
| console.error("Unable to determine parent origin. OSLC selection response not sent."); | |
| } |
|
|
||
| <p id="loadingMessage" style="display: none;">Pondering your search. Please stand by ...</p> | ||
|
|
||
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@1/css/pico.min.css"> |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SRI hash is for HTMX version 2.0.4, but there's no corresponding integrity check for the PicoCSS CDN link. While the PicoCSS link uses HTTPS, adding an integrity attribute would provide additional protection against CDN compromise.
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@1/css/pico.min.css"> | |
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@picocss/pico@1/css/pico.min.css" integrity="sha384-+1Qw6QwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQwQw==" crossorigin="anonymous"> |
| log.error("A empty search should return an empty list and not NULL!"); | ||
| throw new WebApplicationException(Status.INTERNAL_SERVER_ERROR); | ||
|
|
||
| return Response.noContent().build(); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the delegate returns null for resources, the code logs an error stating "A empty search should return an empty list and not NULL!" and then returns a 204 No Content response. However, this masks a potential bug in the delegate implementation rather than handling it appropriately. Consider throwing an exception or returning a 500 Internal Server Error to signal that the delegate violated its contract.
| RequestDispatcher rd = httpServletRequest.getRequestDispatcher("/co/oslc/refimpl/rm" + | ||
| "/gen/requirementselector-results.jsp"); | ||
| httpServletRequest.setAttribute("resources", resources); | ||
| rd.forward(httpServletRequest, httpServletResponse); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When forwarding the request to a JSP that returns HTML fragments for HTMX (lines 289-292), the method continues execution after rd.forward() without returning. This means the subsequent code block (lines 310-313) will still be evaluated, potentially causing issues. Add a return statement after the forward() call to prevent fall-through.
| rd.forward(httpServletRequest, httpServletResponse); | |
| rd.forward(httpServletRequest, httpServletResponse); | |
| return null; |
| final List<Requirement> resources = delegate.RequirementSelector(httpServletRequest, | ||
| serviceProviderId, terms); | ||
| if (resources != null) { | ||
| if ("true".equalsIgnoreCase(httpServletRequest.getHeader("hx-request"))) { |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HTMX request detection using a custom header "hx-request" should be case-sensitive checked. The equalsIgnoreCase method could match "TRUE", "True", etc., but HTMX sends lowercase "true". While this is more lenient (which may be desirable), it's worth noting that the official HTMX header value is always lowercase "true".
| if ("true".equalsIgnoreCase(httpServletRequest.getHeader("hx-request"))) { | |
| if ("true".equals(httpServletRequest.getHeader("hx-request"))) { |
| <button type="button" onclick="search( '<%= selectionUri %>' )">Search</button> | ||
| <input type="search" id="searchTerms" name="terms" placeholder="Begin typing to search" | ||
| autofocus autocomplete="off" | ||
| hx-trigger="input changed delay:120ms, keyup[key=='Enter'], load" |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HTMX trigger configuration 'input changed delay:120ms, keyup[key=='Enter'], load' includes a 'load' event which will fire when the page loads, even when the search input is empty. This will cause an unnecessary request to the server on page load. Consider removing 'load' from the trigger if you don't want to perform a search with empty terms on initial page load.
| hx-trigger="input changed delay:120ms, keyup[key=='Enter'], load" | |
| hx-trigger="input changed delay:120ms, keyup[key=='Enter']" |
| // this is really bad, we should not be mixing HTML and JSON without content | ||
| // negotiation |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on lines 294-295 acknowledges that mixing HTML and JSON responses without proper content negotiation is "really bad", but this approach is still implemented. This creates technical debt and violates REST API principles. Consider refactoring to use proper Accept header negotiation or creating separate endpoints for HTML and JSON responses.
| onEvent : function(name, evt) { | ||
| var elt = evt.detail.elt; | ||
| if(name === "htmx:beforeRequest") { | ||
| elt.setAttribute("aria-busy", "true") | ||
| } else if(name === "htmx:afterRequest" ) { | ||
| elt.removeAttribute("aria-busy") |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JavaScript variables 'elt', 'name', and 'evt' use inconsistent naming conventions. The code uses 'elt' (abbreviated) alongside 'name' and 'evt' (also abbreviated). Consider using more descriptive names like 'element', 'eventName', and 'event' for better code clarity, or at least be consistent with abbreviation style.
| onEvent : function(name, evt) { | |
| var elt = evt.detail.elt; | |
| if(name === "htmx:beforeRequest") { | |
| elt.setAttribute("aria-busy", "true") | |
| } else if(name === "htmx:afterRequest" ) { | |
| elt.removeAttribute("aria-busy") | |
| onEvent : function(eventName, event) { | |
| var element = event.detail.elt; | |
| if(eventName === "htmx:beforeRequest") { | |
| element.setAttribute("aria-busy", "true") | |
| } else if(eventName === "htmx:afterRequest" ) { | |
| element.removeAttribute("aria-busy") |
| // we need to return HTML fragment | ||
| RequestDispatcher rd = httpServletRequest.getRequestDispatcher("/co/oslc/refimpl/rm" + | ||
| "/gen/requirementselector-results.jsp"); | ||
| httpServletRequest.setAttribute("resources", resources); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's inconsistent indentation on line 291 where httpServletRequest.setAttribute has extra leading whitespace compared to the surrounding lines. This creates visual inconsistency in the code block.
| httpServletRequest.setAttribute("resources", resources); | |
| httpServletRequest.setAttribute("resources", resources); |
| <a href="${requirement.about}" | ||
| onclick="sendOslcSelectionPostMessage(this, event)" | ||
| >${requirement.title}</a> |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirement.title and requirement.about fields are rendered directly into HTML without any escaping, while Requirement instances are populated from unsanitized client input in RestDelegate.createRequirement. A malicious client can create a requirement whose title contains HTML/JavaScript so that when this selection dialog is opened, the injected script executes in the browser (stored XSS) and can act with the privileges of the RM provider UI. Ensure that user-controlled fields are properly encoded for HTML text and attribute contexts before rendering (for example, by using JSP/JSTL escaping utilities) so that they cannot inject markup or script.
| <a href="${requirement.about}" | |
| onclick="sendOslcSelectionPostMessage(this, event)" | |
| >${requirement.title}</a> | |
| <a href="<c:out value='${requirement.about}'/>" | |
| onclick="sendOslcSelectionPostMessage(this, event)" | |
| ><c:out value="${requirement.title}"/></a> |
597ab05 to
4950ac7
Compare
4950ac7 to
48eec8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
48eec8b to
9ca2eb4
Compare
I spent 3 hours updating Angular 10 to 11 yesterday and the migration is not fully complete. Mind you, the current version of Angular is 19 and the recommendation is to upgrade only one major version at a time.
Now I spent just 2 hours to write a complete delegated dialog implementation using HTMX instead of making AJAX requests and using Angular to dynamically render results.
Instead, HTMX is a hypermedia framework that is declaratively configured to make HTTP requests and place responses into the HTML DOM. OSLC, being an LDP REST API built with hypermedia principles at heart is perfectly aligned with HTMX. A lot of code can be removed as a result.
One of the best things is that HTMX does not intend to make breaking change like modern frontend frameworks and be more like jQuery in terms of stability. I think it's a perfect fit for the refimpl that needs to reduce the maintenance burden.
Screen.Recording.2025-01-18.at.14.23.29.mov