Performance: slow edit specialists page#2333
Open
LiamStanziani wants to merge 7 commits intohotfix/01232026from
Open
Performance: slow edit specialists page#2333LiamStanziani wants to merge 7 commits intohotfix/01232026from
LiamStanziani wants to merge 7 commits intohotfix/01232026from
Conversation
… into specialists list calls
Move specialist data loading out of inline JSP scriptlets into a dedicated SpecialistList2Action that serves JSON via fetch() POST. The JSP pages now load a lightweight shell with spinner, then fetch and batch-render rows client-side using requestAnimationFrame.
Reviewer's GuideThis PR refactors the specialists configuration UI to load specialist data from a new JSON Struts action backed by a lightweight DTO projection, and batch-renders the table client-side, eliminating large server-side JSP loops and reducing memory and payload size. Sequence diagram for specialist list JSON loading and client-side renderingsequenceDiagram
actor User
participant Browser
participant EditSpecialistsJSP
participant specialistListRenderer_js as SpecialistListRendererJS
participant SpecialistList2Action
participant ProfessionalSpecialistDaoImpl
participant ServiceSpecialistsDao
participant Database
User->>Browser: Navigate to EditSpecialists URL
Browser->>EditSpecialistsJSP: HTTP GET /EditSpecialists.jsp
EditSpecialistsJSP-->>Browser: HTML shell with spinner and JS
Browser->>Browser: Parse HTML and load JS
Browser->>specialistListRenderer_js: renderSpecialistList(config)
specialistListRenderer_js->>SpecialistList2Action: POST /SpecialistList.do?method=getSpecialists
activate SpecialistList2Action
SpecialistList2Action->>ProfessionalSpecialistDaoImpl: findAllListDTOs()
activate ProfessionalSpecialistDaoImpl
ProfessionalSpecialistDaoImpl->>Database: JPQL SELECT NEW SpecialistListDTO(...)
Database-->>ProfessionalSpecialistDaoImpl: List<SpecialistListDTO>
deactivate ProfessionalSpecialistDaoImpl
SpecialistList2Action-->>specialistListRenderer_js: JSON [[id,name,address,phone,fax],...]
deactivate SpecialistList2Action
specialistListRenderer_js->>Browser: Batch create rows with DocumentFragment
Browser->>Browser: Insert 1000 row batches into tbody
Browser->>Browser: Hide spinner and show interactive table
%% Service specific variant
User->>Browser: Navigate to DisplayService URL
Browser->>DisplayServiceJSP: HTTP GET /DisplayService.jsp
DisplayServiceJSP-->>Browser: HTML shell with spinner and JS
Browser->>specialistListRenderer_js: renderSpecialistList(config with serviceId)
specialistListRenderer_js->>SpecialistList2Action: POST /SpecialistList.do?method=getSpecialistsForService&serviceId
activate SpecialistList2Action
SpecialistList2Action->>ProfessionalSpecialistDaoImpl: findAllListDTOs()
activate ProfessionalSpecialistDaoImpl
ProfessionalSpecialistDaoImpl->>Database: JPQL SELECT NEW SpecialistListDTO(...)
Database-->>ProfessionalSpecialistDaoImpl: List<SpecialistListDTO>
deactivate ProfessionalSpecialistDaoImpl
SpecialistList2Action->>ServiceSpecialistsDao: findByServiceId(serviceId)
activate ServiceSpecialistsDao
ServiceSpecialistsDao->>Database: SELECT ServiceSpecialists by serviceId
Database-->>ServiceSpecialistsDao: List<ServiceSpecialists>
deactivate ServiceSpecialistsDao
SpecialistList2Action-->>specialistListRenderer_js: JSON [[id,name,address,phone,fax,checked],...]
deactivate SpecialistList2Action
specialistListRenderer_js->>Browser: Batch create rows with checkbox state
Browser->>Browser: Insert 1000 row batches into tbody and hide spinner
Updated class diagram for specialist listing and renderingclassDiagram
class SpecialistListDTO {
+Integer id
+String firstName
+String lastName
+String professionalLetters
+String streetAddress
+String phoneNumber
+String faxNumber
+SpecialistListDTO(Integer id, String firstName, String lastName, String professionalLetters, String streetAddress, String phoneNumber, String faxNumber)
+Integer getId()
+String getFirstName()
+String getLastName()
+String getProfessionalLetters()
+String getStreetAddress()
+String getPhoneNumber()
+String getFaxNumber()
}
class ProfessionalSpecialistDao {
<<interface>>
+List findAll()
+List findAllListDTOs()
+List findByEDataUrlNotNull()
+List findByFullName(String lastName, String firstName)
}
class ProfessionalSpecialistDaoImpl {
-EntityManager entityManager
+List findAll()
+List findAllListDTOs()
+List findByEDataUrlNotNull()
+List findByFullName(String lastName, String firstName)
}
class ServiceSpecialistsDao {
+List findByServiceId(Integer serviceId)
}
class SpecialistList2Action {
-HttpServletRequest request
-HttpServletResponse response
-SecurityInfoManager securityInfoManager
-ProfessionalSpecialistDao professionalSpecialistDao
-static ObjectMapper MAPPER
+String execute()
-String getSpecialists()
-String getSpecialistsForService()
-Set loadAssignedSpecialistIds(Integer serviceId)
-String buildDisplayName(SpecialistListDTO dto)
-void writeJsonResponse(ArrayNode data)
-String handleServerError(String logMessage, Exception e)
}
class EctConDisplayServiceUtil {
-ConsultationServiceDao consultationServiceDao
+Vector serviceName
+Vector serviceId
+String getServiceDesc(String serId)
+void estServicesVectors()
}
class EditSpecialistsJSP {
+tbodyId specialistBody
+uses renderSpecialistList_config
}
class DisplayServiceJSP {
+tbodyId specialistBody
+String serviceId
+uses renderSpecialistList_config
}
class specialistListRenderer_js {
+void renderSpecialistList(Object config)
+void fetchJson(String url, String csrfTokenName, String csrfTokenValue)
+void renderInBatches(Array data, String tbodyId, Object buildRow)
}
ProfessionalSpecialistDao <|.. ProfessionalSpecialistDaoImpl
SpecialistListDTO <.. ProfessionalSpecialistDaoImpl : JPQL_constructor_projection
SpecialistList2Action --> ProfessionalSpecialistDao : uses
SpecialistList2Action --> ServiceSpecialistsDao : uses
SpecialistList2Action --> SpecialistListDTO : builds_display_rows
EctConDisplayServiceUtil --> ConsultationServiceDao : uses
EditSpecialistsJSP --> specialistListRenderer_js : calls_renderSpecialistList
DisplayServiceJSP --> specialistListRenderer_js : calls_renderSpecialistList
specialistListRenderer_js --> SpecialistList2Action : AJAX_POST_JSON
DisplayServiceJSP --> EctConDisplayServiceUtil : getServiceDesc_only
File-Level Changesplaceholder that is populated via JavaScript.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis pull request optimizes specialist list page loading by converting from server-side rendering to client-side AJAX-based rendering with batch processing. It introduces a lightweight DTO, a new JSON API endpoint, and updates JSP pages to fetch and render specialist data asynchronously in batches rather than loading all data server-side. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant JSP
participant SpecialistList2Action
participant ProfessionalSpecialistDao
participant Database
Browser->>JSP: Load EditSpecialists page
activate JSP
JSP-->>Browser: Page with empty tbody + init script
deactivate JSP
Browser->>Browser: Execute IIFE script
Browser->>SpecialistList2Action: POST /SpecialistList.do?method=getSpecialists
activate SpecialistList2Action
SpecialistList2Action->>ProfessionalSpecialistDao: findAllListDTOs()
activate ProfessionalSpecialistDao
ProfessionalSpecialistDao->>Database: Query lightweight projections
Database-->>ProfessionalSpecialistDao: SpecialistListDTO list
deactivate ProfessionalSpecialistDao
SpecialistList2Action-->>Browser: JSON [id, name, address, phone, fax]
deactivate SpecialistList2Action
Browser->>Browser: Parse JSON response
Browser->>Browser: Batch render 1000 rows per frame
Browser->>Browser: requestAnimationFrame loop
Browser->>Browser: Append rows to specialistBody
Browser->>Browser: Hide spinner when complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Collaborator
Author
|
@SourceryAI review |
Collaborator
Author
|
@coderabbitai review |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the performance and user experience of the Edit Specialists and Display Service pages. By moving from server-side rendering of large specialist datasets to a more efficient client-side approach with data projection and asynchronous batch rendering, the system now handles extensive data volumes with reduced memory footprint, smaller network payloads, and faster interactive page loads. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
✅ Actions performedReview triggered.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- SpecialistList2Action stores HttpServletRequest/Response and Spring beans in instance fields, which can be problematic if the action is reused across threads; consider resolving them per-request inside execute()/helper methods or via proper DI instead of as mutable fields.
- The batch-rendering fetch logic in EditSpecialists.jsp and DisplayService.jsp is nearly identical; consider extracting the common JavaScript into a shared function or script include to reduce duplication and keep future changes in one place.
- When an invalid
methodparameter is passed to SpecialistList2Action, the action currently returns NONE without informing the client; consider returning an explicit 4xx error (e.g., 400) so callers can distinguish misconfiguration from an empty list or success.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SpecialistList2Action stores HttpServletRequest/Response and Spring beans in instance fields, which can be problematic if the action is reused across threads; consider resolving them per-request inside execute()/helper methods or via proper DI instead of as mutable fields.
- The batch-rendering fetch logic in EditSpecialists.jsp and DisplayService.jsp is nearly identical; consider extracting the common JavaScript into a shared function or script include to reduce duplication and keep future changes in one place.
- When an invalid `method` parameter is passed to SpecialistList2Action, the action currently returns NONE without informing the client; consider returning an explicit 4xx error (e.g., 400) so callers can distinguish misconfiguration from an empty list or success.
## Individual Comments
### Comment 1
<location path="src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java" line_range="75" />
<code_context>
+ return getSpecialistsForService();
+ }
+
+ MiscUtils.getLogger().warn("SpecialistList2Action: invalid method parameter: " + method);
+ return NONE;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider returning an explicit 400 status for invalid `method` values instead of just `NONE`.
Right now, an invalid/missing `method` just logs a warning and returns `NONE`, which will go through normal Struts result handling. For this AJAX/JSON endpoint, consider instead sending a `400 Bad Request` (as done for `serviceId` errors in `getSpecialistsForService`) and returning `null`, so clients can clearly distinguish a bad request from a valid empty result.
Suggested implementation:
```java
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
MiscUtils.getLogger().warn("SpecialistList2Action: invalid method parameter: " + method);
return null;
```
1. Ensure that this class has access to the `HttpServletResponse` instance as `response` (for example via `ServletActionContext.getResponse()` or dependency injection), consistent with how `getSpecialistsForService` sets the 400 status.
2. Confirm that `javax.servlet.http.HttpServletResponse` is imported; if it isn't already (it likely is, given the existing 400 handling), add `import javax.servlet.http.HttpServletResponse;` at the top of the file.
</issue_to_address>
### Comment 2
<location path="src/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDaoImpl.java" line_range="61" />
<code_context>
+ @Override
+ public List<SpecialistListDTO> findAllListDTOs() {
+ Query query = entityManager.createQuery(
+ "SELECT NEW ca.openosp.openo.consultation.dto.SpecialistListDTO("
+ + "x.id, x.firstName, x.lastName, x.professionalLetters, "
</code_context>
<issue_to_address>
**suggestion:** Use a TypedQuery for `SpecialistListDTO` instead of a raw `Query` to improve type safety.
You can switch this to `TypedQuery<SpecialistListDTO>` (via cast or a helper on `entityManager.createQuery(...)`) so `getResultList()` is type-safe and you can remove the `@SuppressWarnings("unchecked")` on the call.
Suggested implementation:
```java
@Override
public List<SpecialistListDTO> findAllListDTOs() {
TypedQuery<SpecialistListDTO> query = entityManager.createQuery(
"SELECT NEW ca.openosp.openo.consultation.dto.SpecialistListDTO("
+ "x.id, x.firstName, x.lastName, x.professionalLetters, "
+ "x.streetAddress, x.phoneNumber, x.faxNumber) "
+ "FROM ProfessionalSpecialist x "
+ "ORDER BY x.lastName, x.firstName",
SpecialistListDTO.class
);
List<SpecialistListDTO> results = query.getResultList();
return results;
}
```
1. Add an import for `TypedQuery` at the top of `ProfessionalSpecialistDaoImpl.java`, matching your existing JPA package (e.g. `import javax.persistence.TypedQuery;` or `import jakarta.persistence.TypedQuery;`).
2. If there is an unused `import javax.persistence.Query;` (or `jakarta.persistence.Query;`) after this change, remove it to keep imports clean.
</issue_to_address>
### Comment 3
<location path="src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp" line_range="180" />
<code_context>
+ fragment.appendChild(tr);
+ }
+ tbody.appendChild(fragment);
+ if (idx === Math.min(BATCH_SIZE, data.length)) {
+ HideSpin();
+ }
</code_context>
<issue_to_address>
**suggestion:** Hide the spinner in a more explicit way to avoid relying on an index equality edge case.
Using `idx === Math.min(BATCH_SIZE, data.length)` couples spinner visibility to a specific batching/indexing detail, which makes the intent hard to read and fragile to future batching changes. Consider an explicit "first batch done" condition (e.g., a `firstBatchRendered` flag or a dedicated check before starting the loop) and apply the same approach in `DisplayService.jsp` for consistency.
Suggested implementation:
```
tbody.appendChild(fragment);
if (!firstBatchRendered) {
firstBatchRendered = true;
HideSpin();
}
```
1. In `EditSpecialists.jsp`, declare and initialize `firstBatchRendered` in the JavaScript scope that owns the batching/rendering logic, before the loop that processes `data`, for example:
`let firstBatchRendered = false;`
2. Remove any existing `HideSpin()` call that uses the index-based condition:
`if (idx === Math.min(BATCH_SIZE, data.length)) { HideSpin(); }`
so that `HideSpin()` is only called via the new `firstBatchRendered` logic.
3. In `DisplayService.jsp`, apply the same pattern:
- Introduce a `firstBatchRendered` flag in the appropriate scope.
- After the first batch’s `tbody.appendChild(fragment);`, call `HideSpin()` guarded by the flag:
`if (!firstBatchRendered) { firstBatchRendered = true; HideSpin(); }`
- Remove any index/batch-size–based conditions currently used to call `HideSpin()` there.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDaoImpl.java
Outdated
Show resolved
Hide resolved
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp
Outdated
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that addresses a critical performance issue on the specialist list pages. The strategy of decoupling the data loading from the initial page render is well-executed. Using a lightweight DTO projection on the backend, a dedicated JSON endpoint, and client-side batch rendering with requestAnimationFrame is a modern and effective solution. The code is clean and the changes significantly improve both server-side resource usage and client-side perceived performance. I have one minor suggestion to improve the robustness of the display name formatting.
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR improves perceived and actual load time for consultation specialist configuration pages by moving the large specialist list out of server-side JSP rendering into a separate JSON endpoint and client-side batched rendering.
Changes:
- Replaced JSP
<% for %>specialist row rendering with a lightweight shell that fetches specialist data asynchronously and renders in 1,000-row batches. - Added
SpecialistList2ActionStruts2 JSON endpoints backed by a lightweight JPQL constructor projection (SpecialistListDTO). - Removed legacy specialist vector-building logic from
EctConDisplayServiceUtiland updated Struts mappings accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp | Removes inline specialist table generation; adds fetch + batched DOM rendering. |
| src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp | Same as above, plus “checked” state rendering for a specific service. |
| src/main/webapp/oscarEncounter/oscarConsultationRequest/ConsultationFormRequest.jsp | Removes now-unused specialist preloading call. |
| src/main/webapp/WEB-INF/classes/struts.xml | Registers new SpecialistList2Action endpoint. |
| src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java | New JSON action endpoints for specialist list data. |
| src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/EctConDisplayServiceUtil.java | Deletes specialist vector-building methods no longer used by JSPs. |
| src/main/java/ca/openosp/openo/consultation/dto/SpecialistListDTO.java | New DTO for JPQL constructor projection (reduced hydration). |
| src/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDaoImpl.java | Adds JPQL projection query (findAllListDTOs). |
| src/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDao.java | Declares new projection method. |
src/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDaoImpl.java
Show resolved
Hide resolved
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp
Outdated
Show resolved
Hide resolved
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp
Outdated
Show resolved
Hide resolved
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp
Outdated
Show resolved
Hide resolved
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Outdated
Show resolved
Hide resolved
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp (1)
109-127:⚠️ Potential issue | 🔴 CriticalRe-add the delete submit field.
EctConEditSpecialists2Actionstill decides whether to delete by checking the submitteddeletevalue. This form now renders only the checkboxes, so the delete branch is no longer reachable and row deletion silently stops working.Suggested fix
- <td><form action="${pageContext.request.contextPath}/oscarEncounter/EditSpecialists.do" method="post"> + <td><form action="${pageContext.request.contextPath}/oscarEncounter/EditSpecialists.do" method="post"> + <fmt:message key="oscarEncounter.oscarConsultationRequest.config.EditSpecialists.btnDeleteSpecialist" var="deleteSpecialistLabel"/> ... <table> @@ <tbody id="specialistBody"></tbody> </table> + <input type="submit" name="delete" value="${deleteSpecialistLabel}"> </form></td>Also applies to: 124-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp` around lines 109 - 127, The form in EditSpecialists.jsp no longer includes the submit field named "delete" that EctConEditSpecialists2Action uses to detect delete actions, so deletion no longer works; restore a submit input (e.g., <input type="submit" name="delete" value="...">) inside the form so the action receives the "delete" parameter when users trigger deletions, ensuring the delete branch in EctConEditSpecialists2Action is reachable (also add it back where the other submit controls at lines ~124-125 were removed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/ca/openosp/openo/consultation/dto/SpecialistListDTO.java`:
- Around line 28-57: The public getters in SpecialistListDTO (getId,
getFirstName, getLastName, getProfessionalLetters, getStreetAddress,
getPhoneNumber, getFaxNumber) lack JavaDoc; add Javadoc blocks to each public
accessor describing the returned value, include an `@return` with the exact return
type (e.g., "Integer" or "String") and a `@since` tag, and for completeness add an
`@throws` tag only if the method can throw (for these simple getters indicate
"none" or omit if your project's style permits); ensure the Javadoc follows the
repo standard used for the constructor so each getter has full public
documentation.
In
`@src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java`:
- Around line 75-76: The warning log concatenates the raw request parameter
`method` in SpecialistList2Action which risks log injection; sanitize it before
logging by encoding with Encode.forJava(method) (from the OWASP Encoder) and use
the encoded value in the MiscUtils.getLogger().warn call (i.e., replace the
direct `method` concatenation with the encoded result), ensuring any necessary
import is added and nulls handled consistently in the SpecialistList2Action
class.
In
`@src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp`:
- Around line 194-196: The serviceId is injected unencoded into an HTML value
attribute and into the fetch URL, creating XSS risk; update the HTML input/value
where serviceId is rendered (the value="<%=serviceId %>") to use
Encode.forHtmlAttribute(serviceId) and update the fetch call building the URL
(the fetch("<%= request.getContextPath() %>/...&serviceId=<%=serviceId %>")
fragment) to use a JavaScript/URI-safe encoding such as
Encode.forJavaScriptBlock(serviceId) or Encode.forUriComponent(serviceId) so the
rendered string is safe in both the HTML attribute and JavaScript URL contexts.
---
Outside diff comments:
In
`@src/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp`:
- Around line 109-127: The form in EditSpecialists.jsp no longer includes the
submit field named "delete" that EctConEditSpecialists2Action uses to detect
delete actions, so deletion no longer works; restore a submit input (e.g.,
<input type="submit" name="delete" value="...">) inside the form so the action
receives the "delete" parameter when users trigger deletions, ensuring the
delete branch in EctConEditSpecialists2Action is reachable (also add it back
where the other submit controls at lines ~124-125 were removed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aff9804-f8d9-4cbe-8e05-0d66c8cb33ca
📒 Files selected for processing (9)
src/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDao.javasrc/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDaoImpl.javasrc/main/java/ca/openosp/openo/consultation/dto/SpecialistListDTO.javasrc/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/EctConDisplayServiceUtil.javasrc/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.javasrc/main/webapp/WEB-INF/classes/struts.xmlsrc/main/webapp/oscarEncounter/oscarConsultationRequest/ConsultationFormRequest.jspsrc/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jspsrc/main/webapp/oscarEncounter/oscarConsultationRequest/config/EditSpecialists.jsp
💤 Files with no reviewable changes (2)
- src/main/webapp/oscarEncounter/oscarConsultationRequest/ConsultationFormRequest.jsp
- src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/EctConDisplayServiceUtil.java
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Outdated
Show resolved
Hide resolved
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
Not fully ready as I want to do some more testing with recent changes but want to see another AI review pass through when setting it to ready for review |
Review Summary by QodoDecouple specialist list data from JSP into JSON endpoint with client-side batch rendering
WalkthroughsDescription• Decouple specialist list data from server-side JSP rendering into separate JSON endpoint • Implement JPQL constructor projection to fetch only required columns for list display • Replace inline JSP loops with client-side batch rendering using fetch() and requestAnimationFrame • Remove unused specialist vector construction logic from EctConDisplayServiceUtil Diagramflowchart LR
A["Server-side JSP<br/>with inline loops"] -->|Before| B["226MB memory<br/>1.7MB HTML<br/>4s delay"]
C["SpecialistListDTO<br/>projection"] -->|JPQL| D["Lightweight<br/>DTO query"]
D -->|JSON endpoint| E["SpecialistList2Action"]
E -->|fetch POST| F["Client-side<br/>batch rendering"]
F -->|requestAnimationFrame| G["Fast interactive<br/>page load"]
A -.->|Refactored to| C
B -.->|Improves to| G
File Changes1. src/main/java/ca/openosp/openo/commn/dao/ProfessionalSpecialistDao.java
|
Code Review by Qodo
1. Wrong EntityManager unitName
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
SpecialistList2Action, the JSON responses are positional arrays consumed vias[0],s[1], etc.; consider returning objects with named fields instead to make the client code more self-documenting and less brittle to ordering changes. - The new
SpecialistList2Actionendpoints are documented as POST-only but don't currently enforce HTTP method; adding an explicitrequest.getMethod()check and rejecting non-POST requests would better align behavior with the intended contract. - Rather than resolving
HttpServletRequest,HttpServletResponse, and DAOs at field initialization time viaServletActionContext/SpringUtils, consider using Struts2 injection (e.g.,ServletRequestAware/ServletResponseAwareor setter injection) to improve testability and avoid static lookups in your action.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SpecialistList2Action`, the JSON responses are positional arrays consumed via `s[0]`, `s[1]`, etc.; consider returning objects with named fields instead to make the client code more self-documenting and less brittle to ordering changes.
- The new `SpecialistList2Action` endpoints are documented as POST-only but don't currently enforce HTTP method; adding an explicit `request.getMethod()` check and rejecting non-POST requests would better align behavior with the intended contract.
- Rather than resolving `HttpServletRequest`, `HttpServletResponse`, and DAOs at field initialization time via `ServletActionContext`/`SpringUtils`, consider using Struts2 injection (e.g., `ServletRequestAware`/`ServletResponseAware` or setter injection) to improve testability and avoid static lookups in your action.
## Individual Comments
### Comment 1
<location path="src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java" line_range="67-80" />
<code_context>
+ throw new SecurityException("missing required security object (_con)");
+ }
+
+ String method = request.getParameter("method");
+
+ if ("getSpecialists".equals(method)) {
+ return getSpecialists();
+ } else if ("getSpecialistsForService".equals(method)) {
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider validating the HTTP method to match the documented POST-only API contract
`execute()` routes solely on the `method` parameter and will process requests regardless of the actual HTTP verb. To enforce the POST-only contract, add a `request.getMethod()` check and return an appropriate error (e.g., 405 or 400) for non-POST requests. This helps prevent unintended GET usage and reduces CSRF/caching proxy risks.
```suggestion
String httpMethod = request.getMethod();
if (!"POST".equalsIgnoreCase(httpMethod)) {
try {
response.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED, "This endpoint only supports POST");
} catch (IOException e) {
MiscUtils.getLogger().error("Error sending 405 response", e);
}
return null;
}
String method = request.getParameter("method");
if ("getSpecialists".equals(method)) {
return getSpecialists();
} else if ("getSpecialistsForService".equals(method)) {
return getSpecialistsForService();
}
try {
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid or missing method parameter");
} catch (IOException e) {
MiscUtils.getLogger().error("Error sending 400 response", e);
}
return null;
```
</issue_to_address>
### Comment 2
<location path="src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java" line_range="90" />
<code_context>
+ *
+ * @return null (response written directly to output stream)
+ */
+ private String getSpecialists() {
+ try {
+ List<SpecialistListDTO> specialists = professionalSpecialistDao.findAllListDTOs();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the action to extract shared JSON row/response-building helpers and access dependencies via explicit getters instead of instance fields to reduce duplication and hidden state.
You can cut a fair bit of complexity/duplication without changing behavior by extracting the common row-building and response-writing logic, and by making the dependencies more explicit.
### 1. Extract shared row-building / response-writing
`getSpecialists()` and `getSpecialistsForService()` both manually build very similar JSON arrays. You can centralize that logic and make each method a thin wrapper:
```java
private ArrayNode buildRow(SpecialistListDTO dto) {
ArrayNode row = MAPPER.createArrayNode();
row.add(dto.getId());
row.add(buildDisplayName(dto));
row.add(StringUtils.defaultString(dto.getStreetAddress()));
row.add(StringUtils.defaultString(dto.getPhoneNumber()));
row.add(StringUtils.defaultString(dto.getFaxNumber()));
return row;
}
private void writeSpecialistRows(List<SpecialistListDTO> specialists,
Set<String> assignedIds) throws IOException {
ArrayNode array = MAPPER.createArrayNode();
for (SpecialistListDTO dto : specialists) {
ArrayNode row = buildRow(dto);
if (assignedIds != null) {
row.add(assignedIds.contains(dto.getId().toString()));
}
array.add(row);
}
writeJsonResponse(array);
}
```
Then the action methods become smaller and easier to read:
```java
private String getSpecialists() {
try {
List<SpecialistListDTO> specialists = professionalSpecialistDao.findAllListDTOs();
writeSpecialistRows(specialists, null);
return null;
} catch (Exception e) {
return handleServerError("Error loading specialist list", e);
}
}
private String getSpecialistsForService() {
try {
String serviceIdParam = request.getParameter("serviceId");
Integer serviceId = ConversionUtils.fromIntString(serviceIdParam);
if (serviceId == null) {
response.sendError(HttpServletResponse.SC_BAD_REQUEST,
"Missing or invalid serviceId parameter");
return null;
}
List<SpecialistListDTO> specialists = professionalSpecialistDao.findAllListDTOs();
Set<String> assignedIds = loadAssignedSpecialistIds(serviceId);
writeSpecialistRows(specialists, assignedIds);
return null;
} catch (Exception e) {
return handleServerError("Error loading specialist list for service", e);
}
}
```
This directly addresses the duplication and makes it clear that the only behavioral difference is whether `assignedIds` is present.
### 2. Make dependencies explicit and avoid static field initialization
You can reduce implicit complexity by not initializing `request`, `response`, and DAOs at field declaration time. Instead, resolve them where needed or via a small helper. This keeps lifecycle assumptions local:
```java
private HttpServletRequest getRequest() {
return ServletActionContext.getRequest();
}
private HttpServletResponse getResponse() {
return ServletActionContext.getResponse();
}
private SecurityInfoManager getSecurityInfoManager() {
return SpringUtils.getBean(SecurityInfoManager.class);
}
private ProfessionalSpecialistDao getProfessionalSpecialistDao() {
return SpringUtils.getBean(ProfessionalSpecialistDao.class);
}
```
Then update usage accordingly:
```java
@Override
public String execute() {
HttpServletRequest request = getRequest();
if (!getSecurityInfoManager()
.hasPrivilege(LoggedInInfo.getLoggedInInfoFromSession(request), "_con", "r", null)) {
throw new SecurityException("missing required security object (_con)");
}
String method = request.getParameter("method");
// ...
}
```
And in the methods:
```java
private String getSpecialists() {
HttpServletResponse response = getResponse();
try {
List<SpecialistListDTO> specialists = getProfessionalSpecialistDao().findAllListDTOs();
writeSpecialistRows(specialists, null);
return null;
} catch (Exception e) {
return handleServerError("Error loading specialist list", e);
}
}
```
This keeps behavior identical but makes dependencies clearer and avoids hidden state on the action instance.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Show resolved
Hide resolved
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Show resolved
Hide resolved
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
...dern/java/ca/openosp/openo/consultation/dao/ProfessionalSpecialistDaoDTOIntegrationTest.java
Show resolved
Hide resolved
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp
Show resolved
Hide resolved
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 12 files
Confidence score: 3/5
- There is a concrete runtime regression risk in
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp: it referencesspecialistListRenderer.js, but the file is missing, which can cause a 404 and leaverenderSpecialistListundefined. src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.javadispatches based on themethodquery parameter without enforcing HTTP verb checks, creating a plausible CSRF exposure path if GET is exempt from token validation.- Given a user-facing script failure plus a moderate security concern (both with fairly high confidence), this carries some merge risk and merits validation before release.
- Pay close attention to
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jspandsrc/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java- missing client script wiring and request-method handling could cause runtime breakage and unintended state-changing access.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp">
<violation number="1" location="src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp:73">
P2: `specialistListRenderer.js` is referenced but does not exist in the webapp sources, so the script will 404 and `renderSpecialistList` will be undefined at runtime.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java">
<violation number="1" location="src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java:67">
P2: The `execute()` method dispatches on the `method` query parameter without verifying the actual HTTP verb. Since CSRFGuard typically exempts GET from token validation (`UnprotectedMethods=GET`), a GET request to this endpoint bypasses CSRF protection entirely. Add an HTTP method check and return `405 Method Not Allowed` for non-POST requests to enforce the documented POST-only contract.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/main/webapp/oscarEncounter/oscarConsultationRequest/config/DisplayService.jsp
Show resolved
Hide resolved
.../openosp/openo/encounter/oscarConsultationRequest/config/pageUtil/SpecialistList2Action.java
Show resolved
Hide resolved
Collaborator
Author
|
Will need to review the remaining comments and do some testing. |
…orJackson arrayNode creation
Collaborator
Author
|
Good for review at this point |
2 tasks
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
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
Decouple specialist list data from inline JSP rendering into a separate
fetch() POST endpoint with client-side batch rendering.
Fixes #2321
Problem
The Edit Specialists and Display Service pages embed all specialist data
directly in the HTML response via server-side
<% for %>loops. With10k+ specialist records this causes:
Solution
DTO Projection (server):
SELECT NEW SpecialistListDTO(...))fetches only the 7 columns needed for list display, avoiding full
entity hydration of heavy text fields (annotation, eDataOscarKey, etc.)
Separate JSON endpoint (server):
SpecialistList2Actionserves specialist data as JSON via POST,following the same pattern as
ConsultationLookup2ActionandTicklerList2Actionresponse.getWriter()— decouples data payload from page shellBatch rendering (client):
fetch()POST retrieves JSON from the new endpointrequestAnimationFrame+document.createDocumentFragment()rendersrows in batches of 1,000 for instant perceived load
Summary by Sourcery
Introduce a JSON-based specialist list endpoint and client-side rendering to decouple large specialist datasets from JSP pages and improve performance on specialist configuration screens.
New Features:
Enhancements:
Tests:
Summary by cubic
Move specialist data out of JSP into CSRF-protected JSON endpoints with client-side batch rendering to speed up Edit Specialists and Display Service; cuts payload and server memory for much faster interactivity. Fixes #2321.
SpecialistList2Actionwith POST-only endpointsgetSpecialistsandgetSpecialistsForService, CsrfGuard header support,_conread privilege check, and 400/405 error handling; mapped instruts.xml.ProfessionalSpecialistDao#findAllListDTOs()using JPQL constructor projection toSpecialistListDTO(id, names, letters, address, phone, fax); include integration tests.specialistListRenderer.jsfor batchedrequestAnimationFramerendering and CSRF header; updateEditSpecialists.jspandDisplayService.jspto fetch JSON and render checkboxes/links, keeping the spinner until the first batch renders.EctConDisplayServiceUtiland drop related calls (e.g., fromConsultationFormRequest.jsp).Written for commit a0d1175. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor