Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a refused-value API through the Prevention model and PreventionItem, introduces Prevention-level helpers to read refused status and interpret Smoking semantics, and updates an LDCT DRL rule to use the new smoking-status API instead of the previous "never" check. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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)
📝 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 |
Reviewer's GuideRefactors how smoking status is derived from prevention records by exposing the raw refused status through the model, plumbs that status into prevention items, and updates both Java helpers and Drools rules to use a new isCurrentlySmoking check instead of the previous isPreventionNever-based smoking prevention logic. Sequence diagram for LDCT eligibility smoking check using isCurrentlySmokingsequenceDiagram
actor Clinician
participant EMR_UI
participant DroolsEngine
participant prevention_Prevention
participant PreventionItem
Clinician->>EMR_UI: Open LDCT eligibility screen
EMR_UI->>DroolsEngine: Request LDCT eligibility evaluation
DroolsEngine->>prevention_Prevention: isCurrentlySmoking("Smoking")
activate prevention_Prevention
prevention_Prevention->>prevention_Prevention: getRefusedStatus("Smoking")
prevention_Prevention->>PreventionItem: getRefusedStatus()
PreventionItem-->>prevention_Prevention: refusedStatus (0,1,2,...)
prevention_Prevention-->>DroolsEngine: true if status == 0
deactivate prevention_Prevention
DroolsEngine-->>EMR_UI: LDCT eligibility result
EMR_UI-->>Clinician: Display eligibility based on current smoking status
Class diagram for updated prevention smoking status handlingclassDiagram
class commn_model_Prevention {
+int getRefusedRawValue()
}
class prevention_Prevention {
+int getRefusedStatus(String preventionType)
+boolean isCurrentlySmoking(String preventionType)
}
class PreventionItem {
-int refusedStatus
+PreventionItem(Prevention pp)
+int getRefusedStatus()
}
commn_model_Prevention <.. PreventionItem : constructs_from
prevention_Prevention o--> PreventionItem : uses_most_recent
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 refines the logic for the "LDCT 1" prevention rule by removing a specific check related to smoking prevention. This change aims to streamline the rule's evaluation process, potentially simplifying the criteria for this particular medical prevention guideline. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
![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 removes a check from the 'LDCT 1' Drools rule that verifies a patient is not a 'never-smoker'. While a small change, this has a critical impact on the clinical decision support logic. It will cause the system to incorrectly recommend lung cancer screening for non-smokers. My review includes a critical comment to restore this necessary check to ensure the rule targets the correct patient population (high-risk smokers).
| @@ -1162,7 +1162,6 @@ rule "LDCT 1" | |||
| eval( prev.getAgeInYears() >= 55 ) | |||
| eval( prev.getAgeInYears() <= 74 ) | |||
| eval( prev.getNumberOfPreventionType("Smoking") > 0 ) | |||
There was a problem hiding this comment.
Removing the !prev.isPreventionNever("Smoking") check makes this rule trigger for patients who are documented non-smokers. The rule is intended for high-risk smokers, so this change will generate incorrect clinical warnings.
The comment block for the LDCT rules (lines 1150-1153) explicitly states that the logic should check that the patient is not a "never-smoker".
// Precondition (heavy smoker detection): Smoking assessment must be on record AND // not marked as "never" (non-smoker). This uses the Prevention module's "Smoking" type. // A Smoking record exists (getNumberOfPreventionType("Smoking") > 0) AND // the record is NOT marked as never-smoked (!isPreventionNever("Smoking")).
This change makes the implementation inconsistent with the documentation and the clinical purpose of the rule. The check should be restored.
If this change is intentional, the comment block and the rule's warning message should be updated to reflect that it no longer targets only smokers.
eval( prev.getNumberOfPreventionType("Smoking") > 0 )
eval( !prev.isPreventionNever("Smoking") )
|
@claude I have removed the erroneous eval( !prev.isPreventionNever("Smoking") ) as preventions.never is 0 for all these states Please write a public function in prevention.java to give refused status |
|
Claude finished @phc007's task in 4m 28s —— View job Implementation Complete ✓Root cause confirmed: The Smoking prevention type stores the answer in the Smoking
Changes committed (
--- | Branch |
…er column
The Smoking prevention type stores the smoking answer in the `refused`
column (0=Yes/current, 1=No, 2=Previous), not the `never` column.
The `never` column is always 0 for all three smoking states, so the
previous `!isPreventionNever("Smoking")` check was always true and
failed to exclude non-smokers from LDCT recommendations.
Changes:
- commn/model/Prevention.java: add getRefusedRawValue() to expose raw
refused char as int
- PreventionItem.java: add refusedStatus field and getRefusedStatus()
getter, populated from Prevention.getRefusedRawValue()
- prevention/Prevention.java: add getRefusedStatus(String) and
isCurrentlySmoking(String) public methods for DRL use; document the
smoking refused column encoding and why isPreventionNever is incorrect
- prevention.drl: replace removed eval with isCurrentlySmoking("Smoking"),
update LDCT comment block and header API docs to explain the encoding
Co-authored-by: phc007 <phc007@users.noreply.github.com>
PR Review 🔍
|
PR Code Suggestions ✨
|
![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 left some high level feedback:
- The new
getRefusedRawValue()implementation assumesrefusedis always a digit character ('0'–'3'); if the field can be null or contain other values from the database, consider validating and handling unexpected values (e.g., returning-1or throwing) instead of blindly doingrefused - '0'. - In
PreventionItem, you now track bothboolean refusedandint refusedStatus; to avoid state divergence and confusion, consider deriving the boolean from the int or consolidating to a single source of truth with clearer naming (e.g.,statusCodevs.isRefused).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `getRefusedRawValue()` implementation assumes `refused` is always a digit character ('0'–'3'); if the field can be null or contain other values from the database, consider validating and handling unexpected values (e.g., returning `-1` or throwing) instead of blindly doing `refused - '0'`.
- In `PreventionItem`, you now track both `boolean refused` and `int refusedStatus`; to avoid state divergence and confusion, consider deriving the boolean from the int or consolidating to a single source of truth with clearer naming (e.g., `statusCode` vs. `isRefused`).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
🧹 Nitpick comments (1)
src/main/java/io/github/carlos_emr/carlos/prevention/Prevention.java (1)
355-357: ConstrainisCurrentlySmokingto the Smoking type to prevent semantic misuse.As written, any prevention type with status
0would return true. Given the method name, guarding for"Smoking"avoids accidental misuse.💡 Proposed refactor
public boolean isCurrentlySmoking(String preventionType) { - return getRefusedStatus(preventionType) == 0; + if (!"Smoking".equals(preventionType)) { + return false; + } + return getRefusedStatus(preventionType) == 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/github/carlos_emr/carlos/prevention/Prevention.java` around lines 355 - 357, The method isCurrentlySmoking currently accepts any preventionType and returns true for any type with status 0; change it to a no-argument method that specifically checks the Smoking type to avoid semantic misuse by calling getRefusedStatus with the Smoking constant (e.g., isCurrentlySmoking() { return getRefusedStatus("Smoking") == 0; }) or, if you have a PreventionType enum, change the signature to isCurrentlySmoking() and call getRefusedStatus(PreventionType.SMOKING) == 0 so only the Smoking type is evaluated.
🤖 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/io/github/carlos_emr/carlos/commn/model/Prevention.java`:
- Around line 251-253: The getRefusedRawValue method in class Prevention assumes
the char field refused is between '0' and '3'; change it to defensively validate
the refused field: if refused is between '0' and '3' return refused - '0',
otherwise return a safe default (e.g. 0) or clamp to 0–3; update method
getRefusedRawValue to perform the range check on the refused field and return
the validated integer so the method always yields 0–3 as promised.
In `@src/main/java/io/github/carlos_emr/carlos/prevention/PreventionItem.java`:
- Around line 49-51: The field refusedStatus in class PreventionItem is
defaulting to 0 (current smoker) which can misclassify items; change its default
to an explicit unknown sentinel (e.g., -1 or a named constant like
REFUSED_STATUS_UNKNOWN) and update the PreventionItem class to declare that
constant, initialize refusedStatus to it, and ensure all constructors and any
deserialization/hydration paths set refusedStatus explicitly (or leave as the
sentinel) so no instance is implicitly treated as 0; also update any logic that
checks refusedStatus (getters, isRefused/isSmoker methods, or switch handling)
to handle the sentinel case appropriately.
---
Nitpick comments:
In `@src/main/java/io/github/carlos_emr/carlos/prevention/Prevention.java`:
- Around line 355-357: The method isCurrentlySmoking currently accepts any
preventionType and returns true for any type with status 0; change it to a
no-argument method that specifically checks the Smoking type to avoid semantic
misuse by calling getRefusedStatus with the Smoking constant (e.g.,
isCurrentlySmoking() { return getRefusedStatus("Smoking") == 0; }) or, if you
have a PreventionType enum, change the signature to isCurrentlySmoking() and
call getRefusedStatus(PreventionType.SMOKING) == 0 so only the Smoking type is
evaluated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5f21e4c-f1e8-40fd-920a-7bea0eb3a9e3
📒 Files selected for processing (4)
src/main/java/io/github/carlos_emr/carlos/commn/model/Prevention.javasrc/main/java/io/github/carlos_emr/carlos/prevention/Prevention.javasrc/main/java/io/github/carlos_emr/carlos/prevention/PreventionItem.javasrc/main/resources/oscar/oscarPrevention/prevention.drl
| public int getRefusedRawValue() { | ||
| return refused - '0'; | ||
| } |
There was a problem hiding this comment.
Harden raw refused decoding against unexpected DB values.
Line 252 assumes refused is always '0'..'3'. If data is malformed, this returns an out-of-contract value while JavaDoc promises 0–3.
💡 Proposed fix
public int getRefusedRawValue() {
- return refused - '0';
+ if (refused >= '0' && refused <= '3') {
+ return refused - '0';
+ }
+ return -1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/io/github/carlos_emr/carlos/commn/model/Prevention.java` around
lines 251 - 253, The getRefusedRawValue method in class Prevention assumes the
char field refused is between '0' and '3'; change it to defensively validate the
refused field: if refused is between '0' and '3' return refused - '0', otherwise
return a safe default (e.g. 0) or clamp to 0–3; update method getRefusedRawValue
to perform the range check on the refused field and return the validated integer
so the method always yields 0–3 as promised.
| // Raw refused status value (0=active, 1=refused, 2=ineligible, 3=completedExternally). | ||
| // For the "Smoking" type: 0=Yes (current), 1=No (non-smoker), 2=Previous (ex-smoker). | ||
| private int refusedStatus = 0; |
There was a problem hiding this comment.
Initialize refusedStatus to an unknown sentinel, not smoker status.
Line 51 defaulting to 0 means “current smoker.” If this object is created through constructors that don’t hydrate refusedStatus, it can misclassify status and reintroduce false LDCT triggers.
💡 Proposed fix
- private int refusedStatus = 0;
+ private int refusedStatus = -1;📝 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.
| // Raw refused status value (0=active, 1=refused, 2=ineligible, 3=completedExternally). | |
| // For the "Smoking" type: 0=Yes (current), 1=No (non-smoker), 2=Previous (ex-smoker). | |
| private int refusedStatus = 0; | |
| // Raw refused status value (0=active, 1=refused, 2=ineligible, 3=completedExternally). | |
| // For the "Smoking" type: 0=Yes (current), 1=No (non-smoker), 2=Previous (ex-smoker). | |
| private int refusedStatus = -1; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/io/github/carlos_emr/carlos/prevention/PreventionItem.java`
around lines 49 - 51, The field refusedStatus in class PreventionItem is
defaulting to 0 (current smoker) which can misclassify items; change its default
to an explicit unknown sentinel (e.g., -1 or a named constant like
REFUSED_STATUS_UNKNOWN) and update the PreventionItem class to declare that
constant, initialize refusedStatus to it, and ensure all constructors and any
deserialization/hydration paths set refusedStatus explicitly (or leave as the
sentinel) so no instance is implicitly treated as 0; also update any logic that
checks refusedStatus (getters, isRefused/isSmoker methods, or switch handling)
to handle the sentinel case appropriately.
phc007
changed the title
…if-its-positive-for-ldct
|
@claude fix all bot issues and check if LGTM |
|
Claude finished @phc007's task in 2m 37s —— View job Bot Issues Fixed + LGTM Review
Changes MadeThree issues flagged by CodeRabbit, Sourcery, and Penify have been addressed: 1. 2. 3. LGTM Assessment ✅The PR is LGTM with the fixes applied. Summary of what the PR does:
One pre-existing concern not introduced by this PR (out of scope): |
- Harden getRefusedRawValue() with char range guard (returns -1 for unexpected DB values instead of garbage int) - Initialize PreventionItem.refusedStatus to -1 sentinel instead of 0 to avoid false 'current smoker' classification for items created via constructors that do not hydrate from a Prevention record - Guard isCurrentlySmoking() to return false for non-Smoking types, preventing semantic misuse across other prevention categories Co-authored-by: phc007 <phc007@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/io/github/carlos_emr/carlos/prevention/Prevention.java (1)
281-283:⚠️ Potential issue | 🔴 CriticalFix infinite recursion in
isNotPreventionNever.This public method calls itself recursively without a base case, causing
StackOverflowErrorwhen invoked. Change line 282 to call the workingisPreventionNevermethod instead:return !isPreventionNever(preventionType);The method has no test coverage and no internal usages detected, making this untested code path that will crash on invocation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/github/carlos_emr/carlos/prevention/Prevention.java` around lines 281 - 283, The method isNotPreventionNever currently calls itself causing infinite recursion; change its implementation to return the negation of the existing working method isPreventionNever by replacing the self-call with "return !isPreventionNever(preventionType);" so isNotPreventionNever(String preventionType) delegates to isPreventionNever(String) instead of recursing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/io/github/carlos_emr/carlos/prevention/Prevention.java`:
- Around line 281-283: The method isNotPreventionNever currently calls itself
causing infinite recursion; change its implementation to return the negation of
the existing working method isPreventionNever by replacing the self-call with
"return !isPreventionNever(preventionType);" so isNotPreventionNever(String
preventionType) delegates to isPreventionNever(String) instead of recursing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ef084b8-829f-4763-adf8-1d0eede71f13
📒 Files selected for processing (3)
src/main/java/io/github/carlos_emr/carlos/commn/model/Prevention.javasrc/main/java/io/github/carlos_emr/carlos/prevention/Prevention.javasrc/main/java/io/github/carlos_emr/carlos/prevention/PreventionItem.java
|
@claude Outside diff comments:
|
|
Claude finished @phc007's task in 45s —— View job Fixed: infinite recursion in
|
isNotPreventionNever was calling itself instead of isPreventionNever, causing a StackOverflowError on any invocation. Co-authored-by: phc007 <phc007@users.noreply.github.com>
1 participant
User description
Fix smoking status evaluation used by LDCT eligibility rules by replacing reliance on the incorrect 'never' prevention flag with refused-status-based checks.
Description
Fixes smoking trigger for Low Dose CT prevention alert
Related Issues
Fixes #628
How Was This Tested?
Screenshots
Checklist
git commit -s)Summary by Sourcery
Align smoking-related prevention logic with correct refused status semantics for eligibility rules.
New Features:
Bug Fixes:
Enhancements:
Description
PreventionandPreventionItemclasses to handle smoking history more effectively.Changes walkthrough 📝
Prevention.java
Enhance Prevention Model with Refused Status Handlingsrc/main/java/io/github/carlos_emr/carlos/commn/model/Prevention.java
getRefusedRawValue()method to expose raw refused status.Prevention.java
Add Methods for Smoking Status Evaluationsrc/main/java/io/github/carlos_emr/carlos/prevention/Prevention.java
getRefusedStatus(String)andisCurrentlySmoking(String)methods.
PreventionItem.java
Update PreventionItem with Refused Status Logicsrc/main/java/io/github/carlos_emr/carlos/prevention/PreventionItem.java
getRefusedStatus()method to retrieve raw refused status.refusedStatus.prevention.drl
Revise DRL Rules for Smoking Status Checkssrc/main/resources/oscar/oscarPrevention/prevention.drl
isCurrentlySmoking("Smoking")instead ofisPreventionNever("Smoking").Summary by CodeRabbit
New Features
Bug Fixes
Documentation