Enhancement: Removed recently updated prototype usage in documentsInQueue.jsp for fetch, and jQuery to append data#2335
Open
LiamStanziani wants to merge 2 commits intohotfix/01232026from
Conversation
Reviewer's GuideModernizes the showDocLab() AJAX flow in documentsInQueues.jsp by replacing Prototype.js Ajax.Updater with a fetch-based POST request that includes CSRF protection, URL-encoded parameters, native DOM lookups plus jQuery-based HTML insertion, and basic error handling. Sequence diagram for updated showDocLab fetch-based document loadingsequenceDiagram
actor User
participant Browser as Browser_showDocLab
participant Server as Server_JSP
User->>Browser: Click document/lab in queue
Browser->>Browser: showDocLab(docNo, providerNo, searchProviderNo, status, demoName, inQueue, childId)
Browser->>Browser: Determine type DOC/LAB/OTHER
Browser->>Browser: Resolve placeholder or child element via getElementById
alt Target element not found
Browser->>Browser: console.error("showDocLab: target element not found")
Browser-->>User: No content loaded
else Target element found
Browser->>Browser: Build URL based on type
Browser->>Browser: Build URL-encoded body with encodeURIComponent
Browser->>Server: fetch POST /showDocument.jsp or /labDisplayAjax.jsp
Note right of Browser: Headers: Content-Type, X-Requested-With, CSRF token\nCredentials: same-origin
Server-->>Browser: HTTP response with HTML fragment
alt response.ok
Browser->>Browser: response.text()
alt Placeholder exists
Browser->>Browser: jQuery(div).html(html)
else No placeholder
Browser->>Browser: jQuery(div).append(html)
end
Browser->>Browser: focusFirstDocLab()
Browser-->>User: Updated document/lab content displayed
else !response.ok
Browser->>Browser: Throw Error("Failed to load document")
Browser->>Browser: console.error("Error loading document/lab", err)
Browser-->>User: Error state (no update)
end
end
Flow diagram for updated showDocLab control logicflowchart TD
A[start showDocLab] --> B[Normalize docNo and determine type]
B --> C[Resolve placeholder element by id docPlaceholder_docNo]
C --> D[Resolve target div as placeholder or childId element]
D --> E{Is target div found?}
E -- No --> F[Log console.error target element not found]
F --> G[Return from showDocLab]
E -- Yes --> H[Set URL based on type DOC or LAB]
H --> I[Build form body with segmentID, providerNo, searchProviderNo, status, demoName, inQueue]
I --> J[encodeURIComponent for all parameter values]
J --> K[Call fetch with POST, URL, headers including CSRF, same-origin credentials]
K --> L{response.ok?}
L -- No --> M[Throw error Failed to load document]
M --> N[Log console.error Error loading document or lab]
N --> O[End]
L -- Yes --> P[Read response.text as html]
P --> Q{Placeholder exists?}
Q -- Yes --> R["Use jQuery(div).html(html)"]
Q -- No --> S["Use jQuery(div).append(html)"]
R --> T[Call focusFirstDocLab]
S --> T[Call focusFirstDocLab]
T --> O[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis pull request modernizes the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
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 modernizes the 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
|
Collaborator
Author
|
@coderabbitai review |
Collaborator
Author
|
@SourceryAI review |
✅ 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 1 issue, and left some high level feedback:
- The
datastring sent in the POST body is built by simple concatenation; consider usingencodeURIComponentorURLSearchParamsto safely URL-encode each parameter value so special characters indocNo,demoName, etc. don’t break the request. - The error handling currently only logs to
console.error; if this path is user-facing, consider surfacing a minimal inline error state or fallback in the UI so failures aren’t silent from the user’s perspective.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `data` string sent in the POST body is built by simple concatenation; consider using `encodeURIComponent` or `URLSearchParams` to safely URL-encode each parameter value so special characters in `docNo`, `demoName`, etc. don’t break the request.
- The error handling currently only logs to `console.error`; if this path is user-facing, consider surfacing a minimal inline error state or fallback in the UI so failures aren’t silent from the user’s perspective.
## Individual Comments
### Comment 1
<location path="src/main/webapp/oscarMDS/documentsInQueues.jsp" line_range="925-914" />
<code_context>
+ if (!response.ok) { throw new Error('Failed to load document'); }
+ return response.text();
+ })
+ .then(function (html) {
+ if (placeholder) {
+ jQuery(div).html(html);
+ } else {
+ jQuery(div).append(html);
+ }
+ focusFirstDocLab();
+ })
+ .catch(function (err) {
</code_context>
<issue_to_address>
**suggestion:** DOM update logic diverges slightly from previous `Insertion.Bottom` semantics when `div` is missing.
With the new logic, if `div` cannot be resolved (`document.getElementById(childId)` returns `null`), `jQuery(div)` becomes a no-op: the response is fetched but never inserted when both `placeholder` and `div` are null. If this state is possible, consider failing fast (e.g., explicit error/log or early return before the fetch) to make misconfigurations visible instead of silently dropping the content.
Suggested implementation:
```
.then(function (html) {
if (!div) {
console.error('Error loading document/lab: target container element not found', { childId: childId });
return;
}
if (placeholder) {
jQuery(div).html(html);
} else {
jQuery(div).append(html);
}
focusFirstDocLab();
})
```
1. Ensure that `childId` is in scope for this function. If the identifier for the target element is named differently (e.g., `docId` or similar), update the logged object key/value accordingly.
2. If you prefer failing earlier (before the `fetch` call) instead of here, add a corresponding `if (!div) { console.error(...); return; }` guard at the start of the function that triggers this request, keeping the same error message format for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 pull request is a great enhancement, replacing the deprecated Prototype.js Ajax.Updater with the modern fetch API. The switch to POST requests and the addition of CSRF protection are excellent security improvements. I've found one high-severity issue regarding missing URL encoding for the POST data, which could lead to bugs with special characters in patient data. I've also suggested a medium-severity improvement to provide better error feedback to the user in the UI. Overall, this is a solid modernization effort.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/webapp/oscarMDS/documentsInQueues.jsp (1)
912-935: Solid fetch implementation with CSRF protection.The migration from Prototype's Ajax.Updater to native fetch is well-implemented:
- CSRF headers using JSP tags will render correctly at server-time
- POST method with
credentials: 'same-origin'properly maintains session- jQuery DOM insertion preserves inline script execution for PDF rendering and autocomplete functionality
One minor observation: the error handling logs to console but doesn't provide user feedback. If a document fails to load, the placeholder div remains empty with no indication to the user.
💡 Optional: Add minimal user feedback for failed loads
.catch(function (err) { console.error('Error loading document/lab:', err); + if (div) { + jQuery(div).html('<p class="text-danger">Failed to load document. Please refresh the page.</p>'); + } });,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/oscarMDS/documentsInQueues.jsp` around lines 912 - 935, When the fetch in the document loader fails, add minimal user-visible feedback in the same target element instead of leaving it empty: inside the .catch handler update the target element referenced by div/placeholder (use the existing placeholder flag to decide whether to replace or append) with a brief, accessible error message (e.g., "Unable to load document. Please try again.") and preserve the console.error call; ensure the message is inserted via the same jQuery(div) usage so styling/behavior remains consistent and consider keeping focusFirstDocLab() behavior unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/webapp/oscarMDS/documentsInQueues.jsp`:
- Around line 912-935: When the fetch in the document loader fails, add minimal
user-visible feedback in the same target element instead of leaving it empty:
inside the .catch handler update the target element referenced by
div/placeholder (use the existing placeholder flag to decide whether to replace
or append) with a brief, accessible error message (e.g., "Unable to load
document. Please try again.") and preserve the console.error call; ensure the
message is inserted via the same jQuery(div) usage so styling/behavior remains
consistent and consider keeping focusFirstDocLab() behavior unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad507cc5-eba3-4d39-b711-0d8ef16a771d
📒 Files selected for processing (1)
src/main/webapp/oscarMDS/documentsInQueues.jsp
There was a problem hiding this comment.
Pull request overview
This PR updates the Documents In Queues JSP to modernize the client-side request used to load document/lab content and to integrate OWASP CSRFGuard token handling for those requests.
Changes:
- Added the OWASP CSRFGuard JSP taglib to enable token rendering.
- Replaced a Prototype
Ajax.Updatercall with afetch-based request that injects the CSRF token via request headers. - Switched DOM lookups from Prototype’s
$()todocument.getElementById()in the updated block.
…URL params, and added request with header field
Review Summary by QodoModernize document loading with fetch API and CSRF protection
WalkthroughsDescription• Replace deprecated Prototype.js Ajax.Updater with modern fetch API • Switch from GET to POST requests with CSRF protection headers • Add parameter encoding and target element validation • Use jQuery for DOM insertion to preserve inline scripts Diagramflowchart LR
A["Prototype.js<br/>Ajax.Updater"] -->|"Replace with"| B["Fetch API<br/>POST Request"]
B -->|"Add security"| C["CSRF Headers<br/>same-origin"]
B -->|"Encode params"| D["encodeURIComponent<br/>all values"]
B -->|"Insert HTML"| E["jQuery .html()/.append()<br/>preserve scripts"]
F["Target element<br/>validation"] -->|"Guard against"| G["Missing div<br/>error handling"]
File Changes1. src/main/webapp/oscarMDS/documentsInQueues.jsp
|
Code Review by Qodo
1. Scripts not executed
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider using
URLSearchParams(e.g.new URLSearchParams({...}).toString()) instead of manually concatenating and encoding thedatastring to make the request body construction more readable and less error-prone when adding/removing parameters. - In the
fetcherror handling, you may want to include the HTTP status/response text in the thrown error (or log it) when!response.okto make troubleshooting backend issues easier than a generic 'Failed to load document' message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using `URLSearchParams` (e.g. `new URLSearchParams({...}).toString()`) instead of manually concatenating and encoding the `data` string to make the request body construction more readable and less error-prone when adding/removing parameters.
- In the `fetch` error handling, you may want to include the HTTP status/response text in the thrown error (or log it) when `!response.ok` to make troubleshooting backend issues easier than a generic 'Failed to load document' message.
## Individual Comments
### Comment 1
<location path="src/main/webapp/oscarMDS/documentsInQueues.jsp" line_range="899" />
<code_context>
+ console.error('showDocLab: target element not found for docNo=' + docNo);
+ return;
+ }
var url = '';
if (type == 'DOC')
url = "<%= request.getContextPath() %>/documentManager/showDocument.jsp";
</code_context>
<issue_to_address>
**suggestion:** Consider guarding against empty or unknown URLs before issuing the fetch request.
Right now, if `checkType(docNo)` returns an unexpected value, `url` remains an empty string and `fetch` will target the current page. Even though this matches the old `Ajax.Updater` behavior, with the new explicit error handling it would be clearer and safer to log and `return` when `url` is falsy, so we don’t silently send requests to the wrong endpoint.
Suggested implementation:
```
var url = '';
if (type == 'DOC')
url = "<%= request.getContextPath() %>/documentManager/showDocument.jsp";
else
url = "";
if (!url) {
console.error('showDocLab: unable to resolve URL for docNo=' + docNo + ', type=' + type);
return;
}
```
If the `fetch` (or equivalent AJAX) call is not immediately after this snippet, ensure that the `url` variable is not reassigned later and that all code paths that lead to a network request go through this guard. If there are other call sites or helper functions that construct URLs in a similar way, consider adding the same `!url` guard pattern there for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 3/5
- There is a concrete user-impacting risk in
src/main/webapp/oscarMDS/documentsInQueues.jsp: ifcheckType(docNo)returns an unrecognized type,urlstays empty andfetch('')targets the current page URL. - This can silently POST form-encoded patient data to the wrong endpoint, which raises a meaningful regression/privacy concern, so this is not a low-risk merge despite being isolated to one file.
- Pay close attention to
src/main/webapp/oscarMDS/documentsInQueues.jsp- ensure unrecognized document types cannot fall through to an empty fetch URL.
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/oscarMDS/documentsInQueues.jsp">
<violation number="1" location="src/main/webapp/oscarMDS/documentsInQueues.jsp:920">
P2: When `checkType(docNo)` returns an unrecognized type, `url` remains an empty string. Calling `fetch('')` resolves to the current page URL (per the Fetch spec), so this will silently POST form-encoded patient data to the current JSP page instead of failing visibly. Add a guard before the `fetch` call to bail out when `url` is falsy.
```js
if (!url) {
console.error('showDocLab: unable to resolve URL for docNo=' + docNo + ', type=' + type);
return;
}
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
LiamStanziani
changed the title
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
Replace deprecated Prototype.js Ajax.Updater with modern fetch API in documentsInQueues.jsp showDocLab() function, switching from GET to POST and adding CSRF protection.
This PR was created to address this review comment: #2305 (review)
Problem
The showDocLab() function in documentsInQueues.jsp used Prototype.js's Ajax.Updater and Insertion.Bottom — deprecated libraries that should be phased out. It also used a GET request to load document/lab content, passing
patient-related parameters (segmentID, providerNo, demoName) in the query string.
Solution
Summary by Sourcery
Modernize document/lab loading in documentsInQueues.jsp by replacing Prototype.js Ajax.Updater with a fetch-based POST request secured with CSRF headers and safer DOM handling.
Enhancements:
Summary by cubic
Replaced
Prototype.jsAjax.Updater in documentsInQueues.jsp with a fetch-based POST that includes CSRF headers. UsesjQueryfor DOM insertion and adds param encoding and a target element check to make loading safer.Refactors
application/x-www-form-urlencoded,same-origincredentials, and headersX-Requested-Withand CSRF via theOWASP CSRFGuardtaglib.$()withdocument.getElementById.jQuery.html()/.append()and addedresponse.okchecks with error logging.Bug Fixes
encodeURIComponentto handle special characters.Written for commit 2ad6bd2. Summary will update on new commits.
Summary by CodeRabbit
Security
Refactor