Mhwc 141 card mooving issue#136
Merged
DeepakKumar471 merged 3 commits intorelease-4.0from Feb 20, 2026
Merged
Conversation
…I/HWC-Mobile-App into mhwc-141-card-mooving-issue # Conflicts: # app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt
Contributor
📝 WalkthroughWalkthroughPersonalDetailsFragment refactored to replace DispatcherProvider-based coroutine dispatching with direct Dispatchers.IO/Main calls. Added explicit duplicate beneficiary check with user alert, consolidated data population logic, implemented retry-on-failure patient persistence with fallback field clearing, and updated PDF generation for API 33 context-awareness using Pair<OutputStream, Uri>. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
DeepakKumar471
had a problem deploying
to
HWC_STAG
GitHub Actions
Failure
DeepakKumar471
had a problem deploying
to
HWC_UAT
GitHub Actions
Error
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt (1)
272-292:⚠️ Potential issue | 🟠 MajorData race on shared
Patientobject + inconsistent duplicate-detection semantics
patienthere is the live object from the adapter's dataset. Mutatingpatient.patientID = existing.patientIDonDispatchers.IOwhile the RecyclerView may concurrently be binding views that read from the same object is a data race. Use a defensive copy (e.g.,patient.copy(patientID = existing.patientID)) before modifying.Additionally, this path performs a duplicate check on
beneficiaryRegID, whereassavePatientFromSearch(the other patient-save path) checksbeneficiaryID. These inconsistent keys mean a patient found by one check may slip through the other, producing duplicate rows.🛠️ Suggested fix
- val patient = benVisitInfo.patient - val regId = patient.beneficiaryRegID + val regId = benVisitInfo.patient.beneficiaryRegID if (regId != null) { val existing = patientDao.getPatientByBenRegId(regId) if (existing == null) { - patientDao.insertPatient(patient) + patientDao.insertPatient(benVisitInfo.patient) } else { - patient.patientID = existing.patientID - patientDao.updatePatient(patient) + patientDao.updatePatient(benVisitInfo.patient.copy(patientID = existing.patientID)) } } else { - patientDao.insertPatient(patient) + patientDao.insertPatient(benVisitInfo.patient) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt` around lines 272 - 292, The code mutates the live adapter Patient object on Dispatchers.IO and uses a different dedupe key than savePatientFromSearch; to fix, create a defensive copy of the patient (e.g., from benVisitInfo.patient) before any mutation or DB operations and operate on that copy (do not assign to patient.patientID on the original), and unify the duplicate-detection key with the rest of the codebase (use the same field checked in savePatientFromSearch — beneficiaryID — instead of beneficiaryRegID) when calling patientDao.getPatientBy... so inserts/updates use the copy and the consistent identifier (update the copy's patientID from the existing row before calling patientDao.updatePatient or insertPatient).
🧹 Nitpick comments (2)
app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt (2)
1307-1320:createPdfForApi33name mismatches its actual API guardThe function is invoked for all devices running API ≥
Build.VERSION_CODES.Q(29), not just API 33. The misleading name could cause future callers to skip the guard and call it unconditionally on older devices. Rename it to e.g.createPdfForApi29PlusorcreatePdfWithMediaStore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt` around lines 1307 - 1320, The call site uses createPdfForApi33 for all devices API >= Q (29) which is misleading; rename the function and all its references to a name that matches the actual API guard (e.g., createPdfForApi29Plus or createPdfWithMediaStore) and update its declaration, usages (including the call in PersonalDetailsFragment where you check Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q), and any related docs/comments so the name accurately reflects it runs for Q/29+ rather than only 33.
421-427: RedundantwithContext(Dispatchers.Main)inside an already-Main coroutine
lifecycleScope.launch { … }without an explicit dispatcher runs onDispatchers.Main.immediate. ThewithContext(Dispatchers.Main)blocks nested insidecollect { … }(lines 421–427, 439–445, 457–463, 475–483) are therefore no-ops and only add noise. If background work needs to be dispatched, addwithContext(Dispatchers.IO)around the DB/IO calls instead.♻️ Proposed fix (representative block — same pattern on all four collectors)
lifecycleScope.launch { viewModel.patientListForNurse?.collect { itemAdapter?.submitList(it.sortedByDescending { item -> item.patient.registrationDate }) patientCount = it.size - withContext(Dispatchers.Main) { - if (!isShowingSearchResults) { - binding.patientListContainer.patientList.adapter = itemAdapter - binding.patientListContainer.patientCount.text = - it.size.toString() + getResultStr(it.size) - } + if (!isShowingSearchResults) { + binding.patientListContainer.patientList.adapter = itemAdapter + binding.patientListContainer.patientCount.text = + it.size.toString() + getResultStr(it.size) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt` around lines 421 - 427, The withContext(Dispatchers.Main) calls inside the collectors in PersonalDetailsFragment.kt are redundant because lifecycleScope.launch runs on Dispatchers.Main.immediate; remove those nested withContext(Dispatchers.Main) blocks (the ones wrapping adapter assignment and patient count updates) and leave the UI updates directly in the collector; if any preceding work in the collector is doing DB/IO, move that work into withContext(Dispatchers.IO) (or off-main coroutine) and keep only the UI update lines (adapter assignment and binding.patientListContainer.patientCount.text = ...) on the main coroutine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt`:
- Around line 1382-1396: The createPdfForApi33 function uses unsafe !! and dead
Objects.requireNonNull calls and lacks an API level annotation; change
createPdfForApi33 to be annotated `@RequiresApi`(Build.VERSION_CODES.Q), remove
the Objects.requireNonNull lines, and replace the pdfUri?.let { ... }!! with
explicit null checks: first call context.contentResolver.insert(...) into pdfUri
and if null throw an IOException with a clear message, then call
context.contentResolver.openOutputStream(pdfUri) into outst and if null throw an
IOException with a clear message, finally return Pair(outst, pdfUri) after the
validated non-null checks so callers get meaningful errors instead of a raw NPE.
- Around line 526-540: The existence check currently only runs when
beneficiaryID != null, allowing duplicates when beneficiaryID is null; update
savePatientFromSearch to also check beneficiaryRegID (use
patientDao.getByBeneficiaryRegId or add one if missing) before inserting and
consider both identifiers when calling patientDao.getBen/generic lookup to
prevent duplicates even if beneficiaryID is null, and replace the hardcoded
dialog title/message in the MaterialAlertDialogBuilder with string resources
(R.string.patient_already_exists and R.string.patient_already_exists_message) so
the same dialog is shown whenever either identifier matches; keep generateUuid()
for new patientID only after both checks pass.
- Around line 640-669: In PersonalDetailsFragment's save flow (where
patientRepo.insertPatient is called, using patientToSave and
patientWithNullForeignKeys), do three things: (1) never swallow exceptions — log
both caught exceptions (e and e2) with the logger/Log before handling UI so
stack traces are preserved; (2) eliminate the TOCTOU duplicate gap by making the
insert atomic: either perform the existence check + insert inside a single
database transaction or use an upsert/insert-with-conflict-resolution provided
by patientRepo (or explicitly query for existing by
beneficiaryID/beneficiaryRegID before deciding to insert/update) and on
SQLiteConstraintException inspect the constraint name/message to decide if it is
a FK violation (then retry with nullified FKs) versus a unique-key/duplicate
violation (then abort or merge rather than creating a new UUID); (3) extract
hardcoded dialog strings ("Error Saving Patient", "Failed to save patient…") to
R.string and use them for the MaterialAlertDialogBuilder.
---
Outside diff comments:
In
`@app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt`:
- Around line 272-292: The code mutates the live adapter Patient object on
Dispatchers.IO and uses a different dedupe key than savePatientFromSearch; to
fix, create a defensive copy of the patient (e.g., from benVisitInfo.patient)
before any mutation or DB operations and operate on that copy (do not assign to
patient.patientID on the original), and unify the duplicate-detection key with
the rest of the codebase (use the same field checked in savePatientFromSearch —
beneficiaryID — instead of beneficiaryRegID) when calling
patientDao.getPatientBy... so inserts/updates use the copy and the consistent
identifier (update the copy's patientID from the existing row before calling
patientDao.updatePatient or insertPatient).
---
Nitpick comments:
In
`@app/src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt`:
- Around line 1307-1320: The call site uses createPdfForApi33 for all devices
API >= Q (29) which is misleading; rename the function and all its references to
a name that matches the actual API guard (e.g., createPdfForApi29Plus or
createPdfWithMediaStore) and update its declaration, usages (including the call
in PersonalDetailsFragment where you check Build.VERSION.SDK_INT >=
Build.VERSION_CODES.Q), and any related docs/comments so the name accurately
reflects it runs for Q/29+ rather than only 33.
- Around line 421-427: The withContext(Dispatchers.Main) calls inside the
collectors in PersonalDetailsFragment.kt are redundant because
lifecycleScope.launch runs on Dispatchers.Main.immediate; remove those nested
withContext(Dispatchers.Main) blocks (the ones wrapping adapter assignment and
patient count updates) and leave the UI updates directly in the collector; if
any preceding work in the collector is doing DB/IO, move that work into
withContext(Dispatchers.IO) (or off-main coroutine) and keep only the UI update
lines (adapter assignment and binding.patientListContainer.patientCount.text =
...) on the main coroutine.
...src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt
Show resolved
Hide resolved
...src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt
Show resolved
Hide resolved
...src/main/java/org/piramalswasthya/cho/ui/commons/personal_details/PersonalDetailsFragment.kt
Show resolved
Hide resolved
sushant-bizbrolly
approved these changes
Feb 20, 2026
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
2 participants
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.
📋 Description
JIRA ID: mHWC-141
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
Conflict resolved
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
Bug Fixes
New Features