US#03: Implemented Display Centre Remapping Status - #1086
Merged
anushasunkada merged 19 commits intoJun 12, 2026
Conversation
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
…/android-registration-client into US2-block-operation # Conflicts: # assets/l10n/app_ar.arb # assets/l10n/app_en.arb # assets/l10n/app_fr.arb # assets/l10n/app_hi.arb # assets/l10n/app_kn.arb # assets/l10n/app_ta.arb # lib/ui/onboard/portrait/operational_tasks.dart # lib/ui/onboard/portrait/registration_tasks.dart
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
4-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate
ArrayListimport.Line 5 duplicates the import statement from line 4.
🔧 Proposed fix
import java.sql.Array; import java.util.ArrayList; -import java.util.ArrayList; import java.util.Arrays;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java` around lines 4 - 5, The file RegistrationConstants.java contains a duplicate import of java.util.ArrayList; remove the redundant import so only one java.util.ArrayList import remains (locate the duplicate import lines near the top of the file and delete the extra entry).
♻️ Duplicate comments (1)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImplTest.java (1)
97-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest documents unimplemented behavior.
This test expects
handleReMapProcessto skip processing whenisMachineRemapped()returns false, but the implementation (line 102-126 inCenterRemapServiceImpl.java) never checks the remap flag. This mismatch was flagged in the implementation review.Either update the implementation to add the guard, or remove this test if unconditional processing is the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImplTest.java` around lines 97 - 102, The test expects handleReMapProcess to be a no-op when the MACHINE_CENTER_CHANGED/global param indicates no remap, but CenterRemapServiceImpl.handleReMapProcess currently ignores that flag; update the implementation to read the global param (or call the existing isMachineRemapped() helper if present) at the start of handleReMapProcess and return immediately when the flag is false (e.g., param equals "false"), ensuring no interactions with sync job repos occur, or alternatively remove the test if unconditional processing is intended.
🧹 Nitpick comments (16)
lib/ui/onboard/home_page.dart (1)
139-141: 💤 Low valueUnused
flowparameter.
_remapBlockedMessageignoresflowand always returnsremap_operation_blocked. Either drop the parameter or use it to vary the message; as-is it's dead weight (and the only caller at Line 165 passesprocess.flow).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/onboard/home_page.dart` around lines 139 - 141, The _remapBlockedMessage function currently ignores its flow parameter; either remove the unused parameter from _remapBlockedMessage and from its caller (where process.flow is passed), or update _remapBlockedMessage to switch on the flow value and return different localized strings (e.g., appLocalizations.remap_operation_blocked vs appLocalizations.remap_operation_blocked_forX) so the message varies by flow; modify the function signature and the caller accordingly (or implement the switch inside _remapBlockedMessage using the flow argument) and ensure you reference the existing _remapBlockedMessage and the caller that passes process.flow when making the change.lib/ui/widgets/remap_pending_activities_dialog.dart (1)
96-120: ⚡ Quick winAvoid pushing on the dialog's own (just-popped) context.
In both
_PendingCard.onTaphandlers you callNavigator.of(context).pop()and thenNavigator.push(context, ...)using the dialog'sBuildContext. Afterpop(), that element is being deactivated, so the subsequentpushis resolving the navigator from a context that is tearing down. It happens to work today (same frame), but it's fragile. Capture the parent navigator before popping.♻️ Suggested change (apply to both handlers)
onTap: () { - Navigator.of(context).pop(); - Navigator.push( - context, - MaterialPageRoute( - builder: (_) => const ApprovePacketsPage()), - ); + final navigator = Navigator.of(context); + navigator.pop(); + navigator.push( + MaterialPageRoute( + builder: (_) => const ApprovePacketsPage()), + ); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/widgets/remap_pending_activities_dialog.dart` around lines 96 - 120, Both onTap handlers in the _PendingCard currently call Navigator.of(context).pop() and then Navigator.push(context, ...) using the dialog's BuildContext which is being torn down; capture the navigator before popping and reuse it for the push. Locate the two _PendingCard onTap callbacks (the ones that navigate to ApprovePacketsPage and ExportPacketsPage), store the NavigatorState (e.g., final navigator = Navigator.of(context)) before calling pop(), call navigator.pop(), then call navigator.push(...) to push the appropriate MaterialPageRoute to ApprovePacketsPage or ExportPacketsPage.assets/l10n/app_kn.arb (1)
196-196: ⚡ Quick winAdd metadata blocks for parameterized strings.
The keys
packets_require_supervisor_approvalandpackets_queued_locallyuse the{count}placeholder but lack the corresponding@metadata blocks that define the placeholder type. This pattern is used elsewhere in the file (e.g.,@number_of_applicationat lines 152-160,@language_mandatoryat lines 221-229) and is a Flutter ARB best practice for proper tooling support and type safety.📋 Suggested metadata blocks
Add after line 196:
"packets_require_supervisor_approval": "{count} ಪ್ಯಾಕೆಟ್ಗಳು ಮೇಲ್ವಿಚಾರಕರ ಅನುಮೋದನೆಗಾಗಿ ಕಾಯುತ್ತಿದ್ದು ಸಿಂಕ್ ಮಾಡುವ ಮೊದಲು ಅನುಮೋದಿಸಬೇಕು.", + "`@packets_require_supervisor_approval`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Add after line 198:
"packets_queued_locally": "{count} ಪ್ಯಾಕೆಟ್ಗಳು ಸ್ಥಳೀಯ ಸಿಂಕ್ರೊನೈಸೇಶನ್ಗಾಗಿ ಬಾಕಿ ಇದ್ದು ಕೇಂದ್ರ ರೀಮ್ಯಾಪ್ ಸಿಂಕ್ರೊನೈಸೇಶನ್ ಪ್ರಾರಂಭಿಸುವ ಮೊದಲು ಸರ್ವರ್ಗೆ ಅಪ್ಲೋಡ್ ಮಾಡಬೇಕು.", + "`@packets_queued_locally`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Also applies to: 198-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/l10n/app_kn.arb` at line 196, The two ARB entries packets_require_supervisor_approval and packets_queued_locally use the {count} placeholder but are missing their metadata (@... ) blocks; add corresponding metadata entries for each key (e.g., "`@packets_require_supervisor_approval`" and "`@packets_queued_locally`") that declare the "count" placeholder with "type": "int" and a brief "description" so Flutter tooling and type safety work correctly; update the ARB file by inserting those metadata blocks immediately after the existing string entries for packets_require_supervisor_approval and packets_queued_locally.assets/l10n/app_en.arb (1)
197-197: ⚡ Quick winAdd metadata blocks for parameterized strings.
The keys
packets_require_supervisor_approvalandpackets_queued_locallyuse the{count}placeholder but lack the corresponding@metadata blocks that define the placeholder type. This pattern is used elsewhere in the file (e.g.,@number_of_applicationat lines 152-160,@language_mandatoryat lines 222-230) and is a Flutter ARB best practice for proper tooling support and type safety.📋 Suggested metadata blocks
Add after line 197:
"packets_require_supervisor_approval": "{count} packets are pending supervisor approval and must be approved before synchronization can proceed.", + "`@packets_require_supervisor_approval`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Add after line 199:
"packets_queued_locally": "{count} packets remain pending local synchronization and must be uploaded to the server before the Center Remap Synchronization can be initiated.", + "`@packets_queued_locally`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Also applies to: 199-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/l10n/app_en.arb` at line 197, Add ARB metadata entries for the parameterized keys packets_require_supervisor_approval and packets_queued_locally: for each key add an accompanying @<key> metadata block that defines the placeholder "count" with type "int" (or "number") and a short description so Flutter tooling knows the parameter type; update the ARB near the existing keys (use the symbols packets_require_supervisor_approval and packets_queued_locally to locate them) and follow the same format as other metadata blocks in the file (e.g., `@number_of_application`) to ensure consistency.assets/l10n/app_fr.arb (1)
196-196: ⚡ Quick winAdd metadata blocks for parameterized strings.
The keys
packets_require_supervisor_approvalandpackets_queued_locallyuse the{count}placeholder but lack the corresponding@metadata blocks that define the placeholder type. This pattern is used elsewhere in the file (e.g.,@number_of_applicationat lines 152-160,@language_mandatoryat lines 221-229) and is a Flutter ARB best practice for proper tooling support and type safety.📋 Suggested metadata blocks
Add after line 196:
"packets_require_supervisor_approval": "{count} paquets sont en attente d'approbation du superviseur et doivent être approuvés avant de procéder à la synchronisation.", + "`@packets_require_supervisor_approval`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Add after line 198:
"packets_queued_locally": "{count} paquets restent en attente de synchronisation locale et doivent être téléchargés vers le serveur avant d'initier la synchronisation de réaffectation du centre.", + "`@packets_queued_locally`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Also applies to: 198-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/l10n/app_fr.arb` at line 196, The two ARB entries packets_require_supervisor_approval and packets_queued_locally are parameterized with {count} but missing corresponding metadata blocks; add `@packets_require_supervisor_approval` and `@packets_queued_locally` metadata entries immediately after each string key that declare the placeholder "count" and its type (e.g., "int" or "number") and a brief description to follow the existing ARB pattern used elsewhere (see `@number_of_application` and `@language_mandatory` for examples).assets/l10n/app_hi.arb (1)
196-196: ⚡ Quick winAdd metadata blocks for parameterized strings.
The keys
packets_require_supervisor_approvalandpackets_queued_locallyuse the{count}placeholder but lack the corresponding@metadata blocks that define the placeholder type. This pattern is used elsewhere in the file (e.g.,@number_of_applicationat lines 152-160,@language_mandatoryat lines 221-229) and is a Flutter ARB best practice for proper tooling support and type safety.📋 Suggested metadata blocks
Add after line 196:
"packets_require_supervisor_approval": "{count} पैकेट पर्यवेक्षक की अनुमोदन की प्रतीक्षा में हैं और सिंक्रनाइज़ेशन से पहले स्वीकृत होने चाहिए।", + "`@packets_require_supervisor_approval`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Add after line 198:
"packets_queued_locally": "{count} पैकेट स्थानीय सिंक्रनाइज़ेशन के लिए लंबित हैं और सेंटर रीमैप सिंक्रनाइज़ेशन शुरू करने से पहले सर्वर पर अपलोड होने चाहिए।", + "`@packets_queued_locally`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Also applies to: 198-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/l10n/app_hi.arb` at line 196, The ARB entries packets_require_supervisor_approval and packets_queued_locally use the {count} placeholder but lack metadata; add corresponding metadata blocks named `@packets_require_supervisor_approval` and `@packets_queued_locally` that define the placeholder "count" with type "int" (and a short description), following the pattern used by other keys (e.g., `@number_of_application`); update the file by inserting these @... metadata blocks immediately after each string entry so tooling and type checking recognize the parameter.assets/l10n/app_ta.arb (1)
188-188: ⚡ Quick winAdd metadata blocks for parameterized strings.
The keys
packets_require_supervisor_approvalandpackets_queued_locallyuse the{count}placeholder but lack the corresponding@metadata blocks that define the placeholder type. This pattern is used elsewhere in the file (e.g.,@number_of_applicationat lines 144-152,@language_mandatoryat lines 213-221) and is a Flutter ARB best practice for proper tooling support and type safety.📋 Suggested metadata blocks
Add after line 188:
"packets_require_supervisor_approval": "{count} பாக்கெட்டுகள் மேற்பார்வையாளரின் அனுமதிக்காக நிலுவையில் உள்ளன மற்றும் ஒத்திசைவுக்கு முன் அனுமதிக்கப்பட வேண்டும்.", + "`@packets_require_supervisor_approval`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Add after line 190:
"packets_queued_locally": "{count} பாக்கெட்டுகள் உள்ளூர் ஒத்திசைவுக்காக நிலுவையில் உள்ளன மற்றும் மைய மறுவரைவு ஒத்திசைவை தொடங்குவதற்கு முன் சேவையகத்தில் பதிவேற்றப்பட வேண்டும்.", + "`@packets_queued_locally`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Also applies to: 190-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/l10n/app_ta.arb` at line 188, The two localized strings packets_require_supervisor_approval and packets_queued_locally include the {count} placeholder but lack ARB metadata describing that placeholder; add corresponding metadata blocks for each key (e.g., an "`@packets_require_supervisor_approval`" and "`@packets_queued_locally`" object) that include a placeholders map defining "count" with its type (integer) and a short description so Flutter tooling knows the placeholder type and usage; locate the entries by the exact key names and insert the metadata blocks immediately after their string entries.assets/l10n/app_ar.arb (1)
197-197: ⚡ Quick winAdd metadata blocks for parameterized strings.
The keys
packets_require_supervisor_approvalandpackets_queued_locallyuse the{count}placeholder but lack the corresponding@metadata blocks that define the placeholder type. This pattern is used elsewhere in the file (e.g.,@number_of_applicationat lines 152-160,@language_mandatoryat lines 222-230) and is a Flutter ARB best practice for proper tooling support and type safety.📋 Suggested metadata blocks
Add after line 197:
"packets_require_supervisor_approval": "{count} حزم في انتظار موافقة المشرف ويجب اعتمادها قبل المتابعة في المزامنة.", + "`@packets_require_supervisor_approval`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Add after line 199:
"packets_queued_locally": "{count} حزم لا تزال معلقة للمزامنة المحلية ويجب تحميلها إلى الخادم قبل بدء مزامنة إعادة تعيين المركز.", + "`@packets_queued_locally`": { + "placeholders": { + "count": { + "type": "int" + } + } + },Also applies to: 199-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/l10n/app_ar.arb` at line 197, The two ARB entries "packets_require_supervisor_approval" and "packets_queued_locally" are parameterized with the {count} placeholder but lack metadata; add corresponding metadata blocks named "`@packets_require_supervisor_approval`" and "`@packets_queued_locally`" that include a description and a placeholders map declaring "count" with type "int" (e.g., "placeholders": { "count": { "type": "int" } }) so Flutter tooling and type checking recognize the parameter. Ensure you place each metadata block immediately after its string entry and mirror the style used by existing blocks like "`@number_of_application`" for consistency.lib/provider/sync_provider.dart (2)
187-187: ⚡ Quick winExtract the remap error code into a named constant.
'KER-SNC-149'is hardcoded here and again inmanualSync(Line 303), while the Android side already uses aRegistrationConstants.CENTRE_REMAP_ERROR_CODEconstant. Duplicating the literal across both methods and layers is fragile — a future code change would require edits in multiple places. Define a single Dart constant and reference it in both spots.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/provider/sync_provider.dart` at line 187, Extract the literal 'KER-SNC-149' into a shared Dart constant (e.g., CENTRE_REMAP_ERROR_CODE) and replace the hardcoded string in both occurrences (the conditional checking masterSync.errorCode in the method where masterSync is used and the manualSync method) to reference that constant; declare the constant in a suitable shared scope (top of lib/provider/sync_provider.dart or a central constants file) and update both comparisons to use CENTRE_REMAP_ERROR_CODE so the code no longer duplicates the literal.
104-128: 💤 Low valueOptional: surface the real step failure cause.
executeRemapStepswallows the native error and returnsfalse, so this loop throws a generic'Remap step $step failed'and_remapSyncErrornever reflects the underlying reason (e.g., network/upload failure on step 2). Consider also resetting_remapSyncError = ''at the start ofperformCenterRemapSyncso a prior failure's message doesn't linger during a retry'sinProgressstate.♻️ Suggested tweak
Future<void> performCenterRemapSync() async { _remapSyncStatus = RemapSyncStatus.inProgress; _remapSyncProgress = 0; + _remapSyncError = ''; notifyListeners();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/provider/sync_provider.dart` around lines 104 - 128, Reset _remapSyncError at the start of performCenterRemapSync and, instead of throwing a generic Exception when executeRemapStep(step) returns false, capture the underlying failure detail from the service and surface it: set _remapSyncError = syncResponseService.lastError ?? 'Remap step $step failed' (or change syncResponseService.executeRemapStep to throw the original error and await it so the catch receives the real exception), then set _remapSyncStatus to failure and notifyListeners; ensure the catch block preserves the detailed message already assigned to _remapSyncError. Refer to performCenterRemapSync, syncResponseService.executeRemapStep, and the _remapSyncError/_remapSyncStatus fields.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/CenterRemapService.java (3)
11-11: ⚡ Quick winInconsistent method naming: "ReMap" vs "Remap".
The method name uses
handleReMapProcesswith mixed-case "ReMap", while the class is namedCenterRemapServicewith "Remap". This is inconsistent and may cause confusion.♻️ Proposed fix for consistency
- void handleReMapProcess(int step) throws Exception; + void handleRemapProcess(int step) throws Exception;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/CenterRemapService.java` at line 11, The method name handleReMapProcess is inconsistent with the class CenterRemapService; rename the method to handleRemapProcess (matching "Remap" casing) and update its declaration in the interface and all implementations/usages (including any overrides, calls, and tests) to the new identifier so signatures (e.g., throws Exception) remain unchanged and compilation stays consistent.
11-13: ⚖️ Poor tradeoffConsider using a more specific exception type.
Methods throw generic
Exception, which makes it harder for callers to handle specific failure scenarios. Consider defining a custom exception type (e.g.,CenterRemapException) or using more specific checked exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/CenterRemapService.java` around lines 11 - 13, The methods in CenterRemapService (handleReMapProcess and startRemapProcess) currently throw the broad java.lang.Exception; replace this with a more specific checked exception (e.g., define a CenterRemapException that extends Exception or a suitable subclass) and update the method signatures to throw CenterRemapException instead of Exception, then update all implementations and callers to throw or handle CenterRemapException (and map underlying causes into it) so callers can distinguish remap errors from other exceptions.
9-22: ⚡ Quick winAdd JavaDoc documentation for the public interface.
The interface and its methods lack documentation explaining their purpose, parameters, return values, and exceptions. This is especially important for a public SPI contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/CenterRemapService.java` around lines 9 - 22, Add JavaDoc to the public SPI interface CenterRemapService and all its public methods: document the interface purpose, behavior and any threading/implementation expectations; for handleReMapProcess(int step) describe the meaning of the step parameter and the circumstances under which it throws Exception; for startRemapProcess() describe when it should be called and what exceptions it may propagate; for boolean methods isMachineRemapped(), isPacketsPendingForProcessing(), isPacketsPendingForEOD(), and isPacketsPendingForReRegister() describe what true/false signify and any timing/consistency guarantees; include `@param`, `@return` and `@throws` tags where applicable and keep descriptions concise and implementation-agnostic.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImpl.java (3)
66-82: ⚖️ Poor tradeoffConstructor has 16 parameters.
While Dagger injection handles this, the large parameter count suggests high coupling. Consider grouping related repositories into a facade or composite if this becomes unwieldy to maintain.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImpl.java` around lines 66 - 82, The CenterRemapServiceImpl constructor currently accepts 16 dependencies (GlobalParamRepository, PacketService, PreRegistrationDataSyncService, RegistrationRepository, SyncJobDefRepository, RegistrationCenterRepository, MachineRepository, TemplateRepository, DynamicFieldRepository, IdentitySchemaRepository, LocationRepository, UserDetailRepository, UserBiometricRepository, UserRoleRepository, AuditManagerService, Context), which is high coupling; create one or more cohesive facade/composite classes (e.g., RepositoryFacade or RegistrationDependencies) that group related repositories/services (for example group RegistrationRepository+RegistrationCenterRepository+SyncJobDefRepository, group user-related repos like UserDetailRepository+UserBiometricRepository+UserRoleRepository, etc.), update CenterRemapServiceImpl constructor to inject these new facades (and keep direct injection for standalone services like PacketService/AuditManagerService/Context), and refactor internal usages in CenterRemapServiceImpl to access repositories via the new facade getters to reduce constructor parameters and improve maintainability.
106-125: ⚖️ Poor tradeoffNo validation of step parameter or guard against out-of-order execution.
The
stepparameter is not validated. Invalid values (negative, zero, > 4) are silently ignored. Additionally, there's no enforcement that steps are executed in sequence or protection against re-running steps. WhilestartRemapProcess()enforces order, direct calls tohandleReMapProcess(step)(e.g., from the Flutter layer) may not.Consider adding validation and state tracking if step ordering is critical.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImpl.java` around lines 106 - 125, Validate the incoming step in handleReMapProcess(int step) and guard against invalid values (<=0 or >4) by logging and returning early; additionally add simple state tracking (e.g., an enum or integer field in CenterRemapServiceImpl like currentRemapStep or remapState) to record the last successfully completed step and reject or queue attempts to run a step out of sequence, and make each step method (disableAllSyncJobs, syncAndUploadAllPendingPackets, deleteRegAndPreRegPackets, cleanUpCenterSpecificData) idempotent or check remapState before executing so repeated calls are safe; update handleReMapProcess to check remapState before invoking the target method, log descriptive errors for invalid or out-of-order requests, and expose a reset/force option if needed for recovery.
232-237: ⚖️ Poor tradeoffReplace deprecated network availability check with
NetworkCapabilities(minSdk=28).
ConnectivityManager.getActiveNetworkInfo()/NetworkInfoare deprecated starting API 29; since the app’sminSdkVersionis 28, API 29+ devices will still hit deprecated code (e.g.,CenterRemapServiceImpl.isNetworkAvailable()at lines 232-237). Use aNetworkCapabilities-based check instead. The same deprecated pattern exists inandroid/app/src/main/java/io/mosip/registration_client/utils/NetworkUtils.java(lines 18-19), so consider updating the shared helper too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImpl.java` around lines 232 - 237, Replace the deprecated NetworkInfo check in CenterRemapServiceImpl.isNetworkAvailable() with a NetworkCapabilities-based check: obtain the ConnectivityManager, call getActiveNetwork(), then getNetworkCapabilities(network) and verify it has NET_CAPABILITY_INTERNET (and/or appropriate transports like TRANSPORT_WIFI/TRANSPORT_CELLULAR) before returning true; handle null ConnectivityManager or null NetworkCapabilities by returning false. Update the shared helper in NetworkUtils (method(s) that currently use getActiveNetworkInfo()/NetworkInfo) to the same pattern so both CenterRemapServiceImpl.isNetworkAvailable() and NetworkUtils use getActiveNetwork() + getNetworkCapabilities().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/IdentitySchemaRepository.java`:
- Around line 54-56: deleteAll() currently calls identitySchemaDao.deleteAll()
but does not remove the on-disk schema/process files created by
saveIdentitySchema/createProcessSpec under context.getFilesDir(), leaving
orphaned files; update deleteAll() to first (or after) fetch the schema/process
records (or scan context.getFilesDir()) to determine filenames (e.g., files
named with prefix "schema_" or process-type names persisted by
createProcessSpec) and delete those files from context.getFilesDir()
before/after calling identitySchemaDao.deleteAll(), handling IO exceptions and
logging failures so orphan files are cleaned up and DB and disk stay in sync.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImpl.java`:
- Around line 174-180: The loop in CenterRemapServiceImpl that calls
packetService.uploadRegistration(reg.getPacketId()) currently logs per-packet
exceptions and continues, swallowing failures; update this method to collect
failed packet IDs (e.g., into a List<String> failedPacketIds) when an exception
occurs instead of only logging, and after the loop either throw a new exception
containing the list of failed IDs or return a result object summarizing
successes and failures; ensure you reference the existing
packetService.uploadRegistration call and the pending Registration iteration so
callers can act on partial failures.
- Line 104: The audit call
auditManagerService.audit(AuditEvent.MACHINE_REMAPPED,
Components.CENTER_MACHINE_REMAP) is being executed unconditionally before the
switch on step and thus logs for invalid steps; move the audit so it only runs
when a valid remap branch executes (i.e., place the audit inside each successful
case that performs machine remap) or add an explicit step validation early in
the method (e.g., check the step value and return/error before calling audit) so
that AuditEvent.MACHINE_REMAPPED is only emitted for actual remap actions in
CenterRemapServiceImpl.
- Around line 217-229: The catch in cleanUpCenterSpecificData swallows
repository deletion failures (registrationCenterRepository, machineRepository,
userDetailRepository, etc.) by only logging them; change the method to propagate
failures by rethrowing the caught exception (or wrap and throw a specific
RuntimeException) so callers know cleanup failed, or alternatively add and set a
boolean/Status flag returned from cleanUpCenterSpecificData that indicates
partial failure and is checked by the caller (ensure callers of
cleanUpCenterSpecificData handle the thrown exception or the returned failure
flag); update the Log.e call to include context before rethrowing so the failure
is both logged and surfaced.
- Around line 102-126: The implementation of handleReMapProcess(int step)
processes steps unconditionally but tests expect it to no-op when the machine is
not remapped; add an early guard that calls isMachineRemapped() at the start of
handleReMapProcess and return immediately (no audit or step processing) if it
returns false, so methods like disableAllSyncJobs(),
syncAndUploadAllPendingPackets(), deleteRegAndPreRegPackets(), and
cleanUpCenterSpecificData() are only invoked when isMachineRemapped() is true.
In `@lib/ui/onboard/home_page.dart`:
- Around line 113-137: In onCentreRemapSync, the async await for
registrationTaskProvider.getPacketUploadedPendingDetails() can allow the widget
to be disposed before showDialog is called; move a mounted check immediately
after the await(s) and before any use of context so both the showDialog branch
and the Navigator.push branch are protected; specifically, after obtaining
pendingUpload and pendingApproval (from registrationTaskProvider and
context.read<ApprovePacketsProvider>()), add "if (!mounted) return;" and then
proceed to call showDialog or Navigator.push (leave the globalProvider.getAudit
call fire-and-forget if needed).
---
Outside diff comments:
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java`:
- Around line 4-5: The file RegistrationConstants.java contains a duplicate
import of java.util.ArrayList; remove the redundant import so only one
java.util.ArrayList import remains (locate the duplicate import lines near the
top of the file and delete the extra entry).
---
Duplicate comments:
In
`@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImplTest.java`:
- Around line 97-102: The test expects handleReMapProcess to be a no-op when the
MACHINE_CENTER_CHANGED/global param indicates no remap, but
CenterRemapServiceImpl.handleReMapProcess currently ignores that flag; update
the implementation to read the global param (or call the existing
isMachineRemapped() helper if present) at the start of handleReMapProcess and
return immediately when the flag is false (e.g., param equals "false"), ensuring
no interactions with sync job repos occur, or alternatively remove the test if
unconditional processing is intended.
---
Nitpick comments:
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImpl.java`:
- Around line 66-82: The CenterRemapServiceImpl constructor currently accepts 16
dependencies (GlobalParamRepository, PacketService,
PreRegistrationDataSyncService, RegistrationRepository, SyncJobDefRepository,
RegistrationCenterRepository, MachineRepository, TemplateRepository,
DynamicFieldRepository, IdentitySchemaRepository, LocationRepository,
UserDetailRepository, UserBiometricRepository, UserRoleRepository,
AuditManagerService, Context), which is high coupling; create one or more
cohesive facade/composite classes (e.g., RepositoryFacade or
RegistrationDependencies) that group related repositories/services (for example
group RegistrationRepository+RegistrationCenterRepository+SyncJobDefRepository,
group user-related repos like
UserDetailRepository+UserBiometricRepository+UserRoleRepository, etc.), update
CenterRemapServiceImpl constructor to inject these new facades (and keep direct
injection for standalone services like
PacketService/AuditManagerService/Context), and refactor internal usages in
CenterRemapServiceImpl to access repositories via the new facade getters to
reduce constructor parameters and improve maintainability.
- Around line 106-125: Validate the incoming step in handleReMapProcess(int
step) and guard against invalid values (<=0 or >4) by logging and returning
early; additionally add simple state tracking (e.g., an enum or integer field in
CenterRemapServiceImpl like currentRemapStep or remapState) to record the last
successfully completed step and reject or queue attempts to run a step out of
sequence, and make each step method (disableAllSyncJobs,
syncAndUploadAllPendingPackets, deleteRegAndPreRegPackets,
cleanUpCenterSpecificData) idempotent or check remapState before executing so
repeated calls are safe; update handleReMapProcess to check remapState before
invoking the target method, log descriptive errors for invalid or out-of-order
requests, and expose a reset/force option if needed for recovery.
- Around line 232-237: Replace the deprecated NetworkInfo check in
CenterRemapServiceImpl.isNetworkAvailable() with a NetworkCapabilities-based
check: obtain the ConnectivityManager, call getActiveNetwork(), then
getNetworkCapabilities(network) and verify it has NET_CAPABILITY_INTERNET
(and/or appropriate transports like TRANSPORT_WIFI/TRANSPORT_CELLULAR) before
returning true; handle null ConnectivityManager or null NetworkCapabilities by
returning false. Update the shared helper in NetworkUtils (method(s) that
currently use getActiveNetworkInfo()/NetworkInfo) to the same pattern so both
CenterRemapServiceImpl.isNetworkAvailable() and NetworkUtils use
getActiveNetwork() + getNetworkCapabilities().
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/CenterRemapService.java`:
- Line 11: The method name handleReMapProcess is inconsistent with the class
CenterRemapService; rename the method to handleRemapProcess (matching "Remap"
casing) and update its declaration in the interface and all
implementations/usages (including any overrides, calls, and tests) to the new
identifier so signatures (e.g., throws Exception) remain unchanged and
compilation stays consistent.
- Around line 11-13: The methods in CenterRemapService (handleReMapProcess and
startRemapProcess) currently throw the broad java.lang.Exception; replace this
with a more specific checked exception (e.g., define a CenterRemapException that
extends Exception or a suitable subclass) and update the method signatures to
throw CenterRemapException instead of Exception, then update all implementations
and callers to throw or handle CenterRemapException (and map underlying causes
into it) so callers can distinguish remap errors from other exceptions.
- Around line 9-22: Add JavaDoc to the public SPI interface CenterRemapService
and all its public methods: document the interface purpose, behavior and any
threading/implementation expectations; for handleReMapProcess(int step) describe
the meaning of the step parameter and the circumstances under which it throws
Exception; for startRemapProcess() describe when it should be called and what
exceptions it may propagate; for boolean methods isMachineRemapped(),
isPacketsPendingForProcessing(), isPacketsPendingForEOD(), and
isPacketsPendingForReRegister() describe what true/false signify and any
timing/consistency guarantees; include `@param`, `@return` and `@throws` tags where
applicable and keep descriptions concise and implementation-agnostic.
In `@assets/l10n/app_ar.arb`:
- Line 197: The two ARB entries "packets_require_supervisor_approval" and
"packets_queued_locally" are parameterized with the {count} placeholder but lack
metadata; add corresponding metadata blocks named
"`@packets_require_supervisor_approval`" and "`@packets_queued_locally`" that
include a description and a placeholders map declaring "count" with type "int"
(e.g., "placeholders": { "count": { "type": "int" } }) so Flutter tooling and
type checking recognize the parameter. Ensure you place each metadata block
immediately after its string entry and mirror the style used by existing blocks
like "`@number_of_application`" for consistency.
In `@assets/l10n/app_en.arb`:
- Line 197: Add ARB metadata entries for the parameterized keys
packets_require_supervisor_approval and packets_queued_locally: for each key add
an accompanying @<key> metadata block that defines the placeholder "count" with
type "int" (or "number") and a short description so Flutter tooling knows the
parameter type; update the ARB near the existing keys (use the symbols
packets_require_supervisor_approval and packets_queued_locally to locate them)
and follow the same format as other metadata blocks in the file (e.g.,
`@number_of_application`) to ensure consistency.
In `@assets/l10n/app_fr.arb`:
- Line 196: The two ARB entries packets_require_supervisor_approval and
packets_queued_locally are parameterized with {count} but missing corresponding
metadata blocks; add `@packets_require_supervisor_approval` and
`@packets_queued_locally` metadata entries immediately after each string key that
declare the placeholder "count" and its type (e.g., "int" or "number") and a
brief description to follow the existing ARB pattern used elsewhere (see
`@number_of_application` and `@language_mandatory` for examples).
In `@assets/l10n/app_hi.arb`:
- Line 196: The ARB entries packets_require_supervisor_approval and
packets_queued_locally use the {count} placeholder but lack metadata; add
corresponding metadata blocks named `@packets_require_supervisor_approval` and
`@packets_queued_locally` that define the placeholder "count" with type "int" (and
a short description), following the pattern used by other keys (e.g.,
`@number_of_application`); update the file by inserting these @... metadata blocks
immediately after each string entry so tooling and type checking recognize the
parameter.
In `@assets/l10n/app_kn.arb`:
- Line 196: The two ARB entries packets_require_supervisor_approval and
packets_queued_locally use the {count} placeholder but are missing their
metadata (@... ) blocks; add corresponding metadata entries for each key (e.g.,
"`@packets_require_supervisor_approval`" and "`@packets_queued_locally`") that
declare the "count" placeholder with "type": "int" and a brief "description" so
Flutter tooling and type safety work correctly; update the ARB file by inserting
those metadata blocks immediately after the existing string entries for
packets_require_supervisor_approval and packets_queued_locally.
In `@assets/l10n/app_ta.arb`:
- Line 188: The two localized strings packets_require_supervisor_approval and
packets_queued_locally include the {count} placeholder but lack ARB metadata
describing that placeholder; add corresponding metadata blocks for each key
(e.g., an "`@packets_require_supervisor_approval`" and "`@packets_queued_locally`"
object) that include a placeholders map defining "count" with its type (integer)
and a short description so Flutter tooling knows the placeholder type and usage;
locate the entries by the exact key names and insert the metadata blocks
immediately after their string entries.
In `@lib/provider/sync_provider.dart`:
- Line 187: Extract the literal 'KER-SNC-149' into a shared Dart constant (e.g.,
CENTRE_REMAP_ERROR_CODE) and replace the hardcoded string in both occurrences
(the conditional checking masterSync.errorCode in the method where masterSync is
used and the manualSync method) to reference that constant; declare the constant
in a suitable shared scope (top of lib/provider/sync_provider.dart or a central
constants file) and update both comparisons to use CENTRE_REMAP_ERROR_CODE so
the code no longer duplicates the literal.
- Around line 104-128: Reset _remapSyncError at the start of
performCenterRemapSync and, instead of throwing a generic Exception when
executeRemapStep(step) returns false, capture the underlying failure detail from
the service and surface it: set _remapSyncError = syncResponseService.lastError
?? 'Remap step $step failed' (or change syncResponseService.executeRemapStep to
throw the original error and await it so the catch receives the real exception),
then set _remapSyncStatus to failure and notifyListeners; ensure the catch block
preserves the detailed message already assigned to _remapSyncError. Refer to
performCenterRemapSync, syncResponseService.executeRemapStep, and the
_remapSyncError/_remapSyncStatus fields.
In `@lib/ui/onboard/home_page.dart`:
- Around line 139-141: The _remapBlockedMessage function currently ignores its
flow parameter; either remove the unused parameter from _remapBlockedMessage and
from its caller (where process.flow is passed), or update _remapBlockedMessage
to switch on the flow value and return different localized strings (e.g.,
appLocalizations.remap_operation_blocked vs
appLocalizations.remap_operation_blocked_forX) so the message varies by flow;
modify the function signature and the caller accordingly (or implement the
switch inside _remapBlockedMessage using the flow argument) and ensure you
reference the existing _remapBlockedMessage and the caller that passes
process.flow when making the change.
In `@lib/ui/widgets/remap_pending_activities_dialog.dart`:
- Around line 96-120: Both onTap handlers in the _PendingCard currently call
Navigator.of(context).pop() and then Navigator.push(context, ...) using the
dialog's BuildContext which is being torn down; capture the navigator before
popping and reuse it for the push. Locate the two _PendingCard onTap callbacks
(the ones that navigate to ApprovePacketsPage and ExportPacketsPage), store the
NavigatorState (e.g., final navigator = Navigator.of(context)) before calling
pop(), call navigator.pop(), then call navigator.push(...) to push the
appropriate MaterialPageRoute to ApprovePacketsPage or ExportPacketsPage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fd3acd6a-7829-4b44-8c51-97fc61f07b94
📒 Files selected for processing (51)
android/app/src/main/java/io/mosip/registration_client/HostApiModule.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/GlobalConfigSettingsApi.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/AuditEvent.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/Components.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/DynamicFieldDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/GlobalParamDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/IdentitySchemaDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocationDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/MachineMasterDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/RegistrationCenterDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/RegistrationDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/SyncJobDefDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/TemplateDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/UserBiometricDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/UserPasswordDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/DynamicFieldRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/GlobalParamRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/IdentitySchemaRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/LocationRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/MachineRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/RegistrationCenterRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/RegistrationRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/SyncJobDefRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/TemplateRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/UserBiometricRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/UserDetailRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/UserRoleRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/MasterDataServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/CenterRemapService.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/CenterRemapServiceImplTest.javaassets/l10n/app_ar.arbassets/l10n/app_en.arbassets/l10n/app_fr.arbassets/l10n/app_hi.arbassets/l10n/app_kn.arbassets/l10n/app_ta.arblib/platform_android/global_config_service_impl.dartlib/platform_android/sync_response_service_impl.dartlib/platform_spi/global_config_service.dartlib/platform_spi/sync_response_service.dartlib/provider/sync_provider.dartlib/ui/onboard/home_page.dartlib/ui/onboard/portrait/mobile_home_page.dartlib/ui/onboard/portrait/operational_tasks.dartlib/ui/onboard/portrait/registration_tasks.dartlib/ui/widgets/center_remap_sync_screen.dartlib/ui/widgets/remap_pending_activities_dialog.dartpigeon/global_config_settings.dart
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
SachinPremkumar
force-pushed
the
US3-Remap-Status
branch
from
June 3, 2026 13:12
37bf6fe to
7bcb0b9
Compare
SachinPremkumar
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
lib/provider/sync_provider.dart (1)
142-145: ⚡ Quick winAvoid duplicating the
'KER-SNC-149'magic string.The remap error code is hardcoded in both
autoSync(Line 142) andmanualSync(Line 258). The backend already defines this asCENTRE_REMAP_ERROR_CODEinRegistrationConstants.java. If the code value ever changes server-side, these two literals would silently diverge. Extract a single Dart constant and reference it in both branches.♻️ Proposed refactor
// Define near the top of the file/class static const String centreRemapErrorCode = 'KER-SNC-149';- if (masterSync.errorCode == 'KER-SNC-149') { + if (masterSync.errorCode == centreRemapErrorCode) { _onRemapDetected(); return; }- if (syncResult.errorCode == 'KER-SNC-149') { + if (syncResult.errorCode == centreRemapErrorCode) { isSyncInProgress = false; _onRemapDetected(); return; }Also applies to: 258-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/provider/sync_provider.dart` around lines 142 - 145, Declare a single constant for the remap error code (e.g., static const String centreRemapErrorCode = 'KER-SNC-149') at the top of the class/file and replace the hardcoded literal comparisons in both autoSync (where masterSync.errorCode == 'KER-SNC-149') and manualSync (the same check around lines where masterSync is inspected) to use centreRemapErrorCode; ensure both branches call _onRemapDetected() and return as before but reference the new constant to avoid duplicate magic strings.lib/ui/onboard/home_page.dart (1)
129-131: 💤 Low value
_remapBlockedMessageignores itsflowargument.The
flowparameter is never used and the method unconditionally returnsremap_operation_blocked. Either drop the parameter and callappLocalizations.remap_operation_blockeddirectly at the call site (Line 155), or implement the intended flow-specific messaging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/onboard/home_page.dart` around lines 129 - 131, The _remapBlockedMessage function currently ignores its flow argument and always returns appLocalizations.remap_operation_blocked; fix this by either removing the unused parameter and calling appLocalizations.remap_operation_blocked directly at the call site (where _remapBlockedMessage is invoked), or implement flow-specific logic inside _remapBlockedMessage (e.g., switch/if on the flow string to return different localized messages) and keep the existing call sites unchanged; update the function signature and all callers accordingly so there are no unused parameters or dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/l10n/app_ar.arb`:
- Line 197: The ARB entries packets_require_supervisor_approval and
packets_queued_locally are missing the required metadata blocks for the {count}
placeholder; add corresponding @... metadata entries for each key (e.g.,
`@packets_require_supervisor_approval` and `@packets_queued_locally`) that define
the placeholder "count" with type "String" and a brief description so Flutter
gen-l10n generates the correct method signatures; verify usage in
remap_pending_activities_dialog.dart (calls at lines 92 and 104) which pass a
String via .toString().
In `@assets/l10n/app_en.arb`:
- Line 197: Add missing ARB metadata entries for the parameterized strings so
Flutter gen-l10n generates String-typed parameters: for the keys
packets_require_supervisor_approval and packets_queued_locally add corresponding
metadata blocks "`@packets_require_supervisor_approval`" and
"`@packets_queued_locally`" that declare the placeholder "count" with "type":
"String" (and a brief "description" if desired) so the generated localization
methods accept String for the count argument used by
remap_pending_activities_dialog.dart.
In `@assets/l10n/app_fr.arb`:
- Line 196: The two localization entries packets_require_supervisor_approval and
packets_queued_locally are using a {count} placeholder but are missing the
required metadata blocks for Flutter gen-l10n; add an
`@packets_require_supervisor_approval` metadata block immediately after the key
and an `@packets_queued_locally` metadata block immediately after its key, each
declaring the placeholder `count` with type `String` (since UI passes
count.toString()), and include a brief description/meaning so gen-l10n generates
method signatures with a String parameter.
In `@assets/l10n/app_ta.arb`:
- Line 188: The two localization entries "packets_require_supervisor_approval"
and "packets_queued_locally" are missing the required metadata blocks for the
{count} placeholder; add an '@' metadata block immediately after each key that
declares the placeholder "count" with type "String" and a brief description
(e.g., what the count represents) so Flutter gen-l10n can generate parameterized
methods compatible with remap_pending_activities_dialog.dart which passes
pendingApprovalCount.toString() and pendingUploadCount.toString().
---
Nitpick comments:
In `@lib/provider/sync_provider.dart`:
- Around line 142-145: Declare a single constant for the remap error code (e.g.,
static const String centreRemapErrorCode = 'KER-SNC-149') at the top of the
class/file and replace the hardcoded literal comparisons in both autoSync (where
masterSync.errorCode == 'KER-SNC-149') and manualSync (the same check around
lines where masterSync is inspected) to use centreRemapErrorCode; ensure both
branches call _onRemapDetected() and return as before but reference the new
constant to avoid duplicate magic strings.
In `@lib/ui/onboard/home_page.dart`:
- Around line 129-131: The _remapBlockedMessage function currently ignores its
flow argument and always returns appLocalizations.remap_operation_blocked; fix
this by either removing the unused parameter and calling
appLocalizations.remap_operation_blocked directly at the call site (where
_remapBlockedMessage is invoked), or implement flow-specific logic inside
_remapBlockedMessage (e.g., switch/if on the flow string to return different
localized messages) and keep the existing call sites unchanged; update the
function signature and all callers accordingly so there are no unused parameters
or dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebd10b55-5dc1-4424-8b7d-df87e8a03e69
📒 Files selected for processing (12)
assets/l10n/app_ar.arbassets/l10n/app_en.arbassets/l10n/app_fr.arbassets/l10n/app_hi.arbassets/l10n/app_kn.arbassets/l10n/app_ta.arblib/platform_android/sync_response_service_impl.dartlib/provider/sync_provider.dartlib/ui/onboard/home_page.dartlib/ui/onboard/portrait/mobile_home_page.dartlib/ui/onboard/portrait/registration_tasks.dartlib/ui/widgets/remap_pending_activities_dialog.dart
✅ Files skipped from review due to trivial changes (2)
- assets/l10n/app_kn.arb
- assets/l10n/app_hi.arb
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/ui/onboard/portrait/mobile_home_page.dart
- lib/ui/onboard/portrait/registration_tasks.dart
- lib/platform_android/sync_response_service_impl.dart
- lib/ui/widgets/remap_pending_activities_dialog.dart
SachinPremkumar
force-pushed
the
US3-Remap-Status
branch
from
June 3, 2026 13:21
7bcb0b9 to
5420ebd
Compare
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
SachinPremkumar
force-pushed
the
US3-Remap-Status
branch
from
June 4, 2026 01:40
5420ebd to
919b613
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ui/onboard/home_page.dart (1)
59-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad the persisted remap flag before relying on
isCenterRemapped.All of the new blocking logic in this page reads
syncProvider.isCenterRemapped, butSyncProviderstarts with_isCenterRemapped = falseand this widget never callscheckCenterRemapState(). After an app restart, a device that was already remapped will therefore render this page as if it were still usable until another sync attempt hitsKER-SNC-149, which defeats the intended gating.Suggested fix
`@override` void initState() { syncProvider = Provider.of<SyncProvider>(context, listen: false); globalProvider = Provider.of<GlobalProvider>(context, listen: false); registrationTaskProvider = Provider.of<RegistrationTaskProvider>(context, listen: false); connectivityProvider = Provider.of<ConnectivityProvider>(context, listen: false); + syncProvider.checkCenterRemapState(); _fetchProcessSpec(); WidgetsBinding.instance.addPostFrameCallback((_) async {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/onboard/home_page.dart` around lines 59 - 73, The page currently reads syncProvider.isCenterRemapped but never loads the persisted remap state on startup; update initState (in HomePage widget) to call syncProvider.checkCenterRemapState() (or the provider method that loads the persisted flag) before any logic that relies on isCenterRemapped (e.g., before _fetchProcessSpec() and UI gating). Do this in initState — either await the check in a WidgetsBinding.instance.addPostFrameCallback or call it synchronously if it reads from local storage — so the persisted _isCenterRemapped value is populated before the page makes decisions based on isCenterRemapped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ui/onboard/home_page.dart`:
- Around line 119-127: The tap handler currently only calls showDialog when
pendingUpload or pendingApproval are > 0, causing a silent no-op when both are
zero; update the handler in home_page.dart (where the remap card tap invokes
RemapPendingActivitiesDialog) to always call showDialog with the same
context/builder/RemapPendingActivitiesDialog parameters instead of gating it
behind the pending counts, so the dialog (which already conditionally shows
pending cards and remap guidance) is displayed even when counters are zero.
---
Outside diff comments:
In `@lib/ui/onboard/home_page.dart`:
- Around line 59-73: The page currently reads syncProvider.isCenterRemapped but
never loads the persisted remap state on startup; update initState (in HomePage
widget) to call syncProvider.checkCenterRemapState() (or the provider method
that loads the persisted flag) before any logic that relies on isCenterRemapped
(e.g., before _fetchProcessSpec() and UI gating). Do this in initState — either
await the check in a WidgetsBinding.instance.addPostFrameCallback or call it
synchronously if it reads from local storage — so the persisted
_isCenterRemapped value is populated before the page makes decisions based on
isCenterRemapped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1df39051-acdc-4d7a-9874-b1210b55735c
📒 Files selected for processing (12)
assets/l10n/app_ar.arbassets/l10n/app_en.arbassets/l10n/app_fr.arbassets/l10n/app_hi.arbassets/l10n/app_kn.arbassets/l10n/app_ta.arblib/platform_android/sync_response_service_impl.dartlib/provider/sync_provider.dartlib/ui/onboard/home_page.dartlib/ui/onboard/portrait/mobile_home_page.dartlib/ui/onboard/portrait/registration_tasks.dartlib/ui/widgets/remap_pending_activities_dialog.dart
✅ Files skipped from review due to trivial changes (1)
- lib/platform_android/sync_response_service_impl.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/ui/onboard/portrait/mobile_home_page.dart
- lib/ui/widgets/remap_pending_activities_dialog.dart
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
…/android-registration-client into US2-block-operation
…mar/android-registration-client into US3-Remap-Status # Conflicts: # lib/ui/onboard/home_page.dart
…ation-client into US3-Remap-Status # Conflicts: # android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/MasterDataServiceImpl.java # assets/l10n/app_ar.arb # assets/l10n/app_en.arb # assets/l10n/app_fr.arb # assets/l10n/app_hi.arb # assets/l10n/app_kn.arb # assets/l10n/app_ta.arb # lib/ui/onboard/home_page.dart
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
…ation-client into US3-Remap-Status # Conflicts: # assets/l10n/app_ar.arb # assets/l10n/app_en.arb # assets/l10n/app_fr.arb # assets/l10n/app_hi.arb # assets/l10n/app_kn.arb # assets/l10n/app_ta.arb # lib/ui/onboard/home_page.dart
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
anushasunkada
approved these changes
Jun 12, 2026
12 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
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.
#1053
Summary by CodeRabbit
New Features
Improvements