fix: Rx previous instructions modal shows nothing ("nada") when clicking *#955
Conversation
…scade + null safety) Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/7dcc0e83-dc5f-45c5-93eb-da129870f7f5 Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Pull request overview
Fixes the Rx “previous instructions” (*) modal failing to open by preventing server-side 500s and ensuring client-side injected scripts don’t abort subsequent execution.
Changes:
- Hardened
DrugDaoImpl.findByParameterby binding the value as a named parameter and allowlisting column names to avoid apostrophe crashes and SQL injection. - Updated
CarlosAjax.insertContentto isolate injected script execution so one failing script doesn’t prevent later scripts from running. - Added null-guards in
RxWriteScript2Action.listPreviousInstructionsanddisplayMedHistory.jspto avoid NPE-driven blank/failed UI.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/webapp/share/javascript/carlos-ajax.js | Wrap injected script execution per-script to avoid cascade aborts. |
| src/main/webapp/oscarRx/displayMedHistory.jsp | Add null guards around session bean / Rx stash item access. |
| src/main/java/io/github/carlos_emr/carlos/prescript/pageUtil/RxWriteScript2Action.java | Guard missing stash item to avoid NPE when building previous-instructions list. |
| src/main/java/io/github/carlos_emr/carlos/commn/dao/DrugDaoImpl.java | Parameterize native SQL value and add a column allowlist for safe dynamic column selection. |
|
@copilot resolve the merge conflicts in this pull request |
Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/cdc981e6-2715-4a3e-91cb-cb773f383afb Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Resolved the merge conflicts and pushed true merge commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot lgtm? |
Code-wise, yes: the merge conflict is resolved in |
|
@copilot are all review comments from all reviewers which are valid review addressed? |
Yes — from the reviewer feedback on this PR, the valid actionable comments appear addressed in |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/5cf2c52e-079b-4cfc-9c04-a3d20fb8433c Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/5cf2c52e-079b-4cfc-9c04-a3d20fb8433c Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/5cf2c52e-079b-4cfc-9c04-a3d20fb8433c Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Applied in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
I wasn’t able to confidently verify this manually because the prescription UI is hard to navigate. There are multiple * controls with different meanings and the set up is unideal. Playwright verification did seem to exercise the intended flow successfully: after staging a medication, clicking the* beside the Instructions field sent listPreviousInstructions and opened the med history popup. I filed a follow-up ticket for the related Rx UI confusion here: #2633 I don’t think that broader UI cleanup needs to block this PR, but it’s worth linking so the remaining verification difficulty is tracked separately. That said, I was unable to verify manually that this was working. |
Uh oh!
There was an error while loading. Please reload this page.