RCF-1293 Updated AndroidManifest for uses-permission and implemented background sync after app is closed#704
Open
MadhuMosip wants to merge 7 commits intomosip:developfrom
Open
RCF-1293 Updated AndroidManifest for uses-permission and implemented background sync after app is closed#704MadhuMosip wants to merge 7 commits intomosip:developfrom
MadhuMosip wants to merge 7 commits intomosip:developfrom
Conversation
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a WorkManager-based sync system: new Application subclass exposing a Dagger AppComponent, SyncScheduler and SyncWorker for job scheduling/execution, rewires MainActivity and sync APIs to use the scheduler, updates storage permissions and removes UploadBackgroundService and exact-alarm permissions. Changes
Sequence Diagram(s)sequenceDiagram
participant App as RegistrationClientApp
participant Main as MainActivity
participant Scheduler as SyncScheduler
participant WM as WorkManager
participant Worker as SyncWorker
participant API as MasterDataSyncApi
participant Batch as BatchJob
App->>Main: app starts / expose AppComponent
Main->>Scheduler: scheduleAllActiveJobs(context)
Scheduler->>WM: enqueue unique OneTimeWorkRequest(jobApiName)
WM->>Worker: start Worker(jobApiName)
Worker->>App: getAppComponent()
Worker->>API: obtain MasterDataSyncApi
Worker->>Scheduler: obtain SyncScheduler
Worker->>API: executeJobByApiName(jobApiName, context, onComplete)
API->>Batch: run job logic
Batch-->>API: job finished
API->>Scheduler: scheduleJob(...) (reschedule if needed)
API-->>Worker: notify completion
Worker-->>WM: return Result.success()/retry()/failure()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/AndroidManifest.xml`:
- Around line 27-29: The <service> declaration for UploadBackgroundService
incorrectly includes android:permission="android.permission.BIND_JOB_SERVICE";
remove this attribute from the UploadBackgroundService entry in
AndroidManifest.xml because UploadBackgroundService extends Service (not
JobService) and is started via explicit Intents (see
MainActivity.createBackgroundTask, MasterDataSyncApi.resetAlarm, and
AuthenticationApi), leaving android:exported="false" intact.
- Around line 4-10: The manifest adds MANAGE_EXTERNAL_STORAGE but the app
currently calls getExternalStoragePublicDirectory() without checking or
requesting the special runtime access; update StorageUtils to check
Environment.isExternalStorageManager() before any shared-storage access and, if
false, either fall back to getExternalFilesDir() or launch the system permission
flow using ACTION_MANAGE_ALL_FILES_ACCESS_PERMISSION to send the user to
settings; ensure any call sites that assume shared storage verify this check
first. Also remove the android:permission="android.permission.BIND_JOB_SERVICE"
attribute from UploadBackgroundService (which extends Service, not JobService)
so the component uses an appropriate permission configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a6d8a627-5a2c-4cff-ade5-a6b98ac3f7a0
📒 Files selected for processing (1)
android/app/src/main/AndroidManifest.xml
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
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)
android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java (1)
32-55:⚠️ Potential issue | 🟠 MajorAvoid storing a strong
Activityon a@Singleton.
BatchJobnow lives for the whole app process, butsetCallbackActivity()still retains the lastActivityinstance. That leaks the old screen across rotation/background work and lets sync callbacks or toasts target a stale destroyed activity. Use aWeakReference<Activity>or clear the callback when the UI goes away.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java` around lines 32 - 55, BatchJob currently holds a strong Activity reference in the activity field and via setCallbackActivity(Activity), which can leak activities because BatchJob is a `@Singleton`; change the activity field to a WeakReference<Activity> (or Optional/nullable cleared reference), update setCallbackActivity(Activity) to store a new WeakReference<>(callbackActivity) (and add a clearCallbackActivity() to null/clear the reference), and update any code that reads activity to dereference the WeakReference safely (check get() != null before using) and skip callbacks/toasts if the Activity has been garbage-collected; ensure methods that previously assumed activity non-null handle a null gracefully.
🧹 Nitpick comments (3)
android/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.java (3)
94-126: Consider adding callback for initialization status feedback.
scheduleAllActiveJobsruns asynchronously in a rawThreadand silently returns early if the machine is not configured (line 98-101). The caller (MainActivity.initializeAppComponent) has no visibility into whether scheduling succeeded or was skipped. For better observability, consider:
- Using a
CompletableFutureor callback to report initialization status- At minimum, adding structured logging with a distinct tag for monitoring/debugging
This is a minor concern since the current behavior may be intentional (silent degradation when unconfigured).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.java` around lines 94 - 126, scheduleAllActiveJobs currently spins a raw Thread and can return silently (e.g., when CenterMachineDto is null), giving callers like MainActivity.initializeAppComponent no visibility; change scheduleAllActiveJobs to accept a callback or return a CompletableFuture (e.g., a CompletableFuture<Void> or CompletableFuture<Boolean>) so callers receive success/failure/skip status, resolve the future (or invoke the callback) on normal completion with the number of scheduled jobs or a skipped flag, and complete exceptionally (or pass an error) on exceptions; update usages such as MainActivity.initializeAppComponent to await or register the callback and add structured Log.i/Log.w/Log.e messages in scheduleAllActiveJobs (and around scheduleJob calls) to provide clear initialization telemetry.
139-154: Code duplication:getExcludedJobIdsandaddJobIdsFromStringappear in bothSyncSchedulerandMasterDataSyncApi.Lines 139-154 duplicate the same logic found in
MasterDataSyncApi.java(lines 745-762). Consider extracting these helpers to a shared utility class to reduce maintenance burden and ensure consistent behavior.♻️ Proposed refactor: Extract to JobExclusionHelper
// New utility class public class JobExclusionHelper { public static Set<String> getExcludedJobIds(GlobalParamRepository globalParamRepository) { Set<String> excluded = new HashSet<>(); addJobIdsFromString(excluded, globalParamRepository.getCachedStringJobsOffline()); addJobIdsFromString(excluded, globalParamRepository.getCachedStringJobsUntagged()); return excluded; } public static void addJobIdsFromString(Set<String> target, String value) { if (value == null || value.trim().isEmpty()) return; for (String jobId : value.split(RegistrationConstants.COMMA)) { String trimmed = jobId.trim(); if (!trimmed.isEmpty()) { target.add(trimmed); } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.java` around lines 139 - 154, The getExcludedJobIds and addJobIdsFromString methods in SyncScheduler duplicate logic found in MasterDataSyncApi; extract them into a shared utility (e.g., JobExclusionHelper) with public static methods getExcludedJobIds(GlobalParamRepository) and addJobIdsFromString(Set<String>, String), then replace SyncScheduler.getExcludedJobIds and MasterDataSyncApi's copies to call JobExclusionHelper.getExcludedJobIds(globalParamRepository) (and JobExclusionHelper.addJobIdsFromString where needed) to remove duplication and centralize behavior while keeping the same method semantics and use of RegistrationConstants.COMMA.
58-66: Method namegetIntervalMillisis misleading—it returns an absolute timestamp, not an interval.The
getIntervalMillis()method inBatchJob.java(line 276-278) callsCronParserUtil.getNextExecutionTimeInMillis(cronExp)and returns that value, which is an absolute next execution timestamp. The method name should begetNextExecutionTimeMillis()to match its actual behavior and avoid confusion. The usage inSyncScheduler.javaline 60-61 is correct (delay = nextExecutionTime - System.currentTimeMillis()), but the misleading method name obscures intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.java` around lines 58 - 66, Rename the misleading BatchJob.getIntervalMillis(...) to BatchJob.getNextExecutionTimeMillis(...) (it currently returns an absolute next-execution timestamp from CronParserUtil.getNextExecutionTimeInMillis(cronExp)), update its declaration in BatchJob.java and all call sites (e.g., SyncScheduler.scheduleJob where batchJob.getIntervalMillis(jobApiName) is used) so callers compute delay as nextExecutionTime - System.currentTimeMillis(), and keep the existing behavior/logging unchanged; ensure no remaining references to getIntervalMillis remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/io/mosip/registration_client/api_services/AuthenticationApi.java`:
- Around line 223-233: The stopAlarmService method currently only cancels
SyncScheduler jobs and misses cleaning up legacy
AlarmManager/UploadBackgroundService PendingIntents; update stopAlarmService in
AuthenticationApi to also cancel the old AlarmManager alarm (recreate the same
PendingIntent used by the legacy UploadBackgroundService and call
AlarmManager.cancel(pendingIntent)) or run a one-time migration that removes
that PendingIntent so legacy alarms stop firing on upgrade; reference
stopAlarmService, syncScheduler, UploadBackgroundService, PendingIntent and
AlarmManager when implementing the additional cleanup.
In `@android/app/src/main/java/io/mosip/registration_client/SyncWorker.java`:
- Around line 54-62: The current code treats a timed-out latch (from
latch.await(SYNC_TIMEOUT_MINUTES, TimeUnit.MINUTES)) as success which suppresses
WorkManager retries and can overlap runs; change the control flow so that if
completed is false you log the timeout and return Result.retry() (and do NOT
call scheduler.scheduleJob(...)) and only call
scheduler.scheduleJob(getApplicationContext(), jobApiName) and return
Result.success() when completed is true; update the branch around latch.await,
SYNC_TIMEOUT_MINUTES, scheduler.scheduleJob, and the final
Result.success()/Result.retry() to reflect this.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java`:
- Around line 40-67: Multiple threads can concurrently restore and write tokens
causing races; modify ensureValidSessionToken to synchronize the
DB-restore-and-save sequence using a shared lock (e.g., a private final Object
restoreLock) so only one thread performs the restore; inside the synchronized
block re-check sessionManager.fetchAuthToken()/isTokenValid(token) to see if
another thread already restored a fresh token before calling
userTokenDao.findByUsername(...) and sessionManager.saveAuthToken(dbToken), and
only write back when the in-memory token is still absent/invalid.
---
Outside diff comments:
In `@android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java`:
- Around line 32-55: BatchJob currently holds a strong Activity reference in the
activity field and via setCallbackActivity(Activity), which can leak activities
because BatchJob is a `@Singleton`; change the activity field to a
WeakReference<Activity> (or Optional/nullable cleared reference), update
setCallbackActivity(Activity) to store a new WeakReference<>(callbackActivity)
(and add a clearCallbackActivity() to null/clear the reference), and update any
code that reads activity to dereference the WeakReference safely (check get() !=
null before using) and skip callbacks/toasts if the Activity has been
garbage-collected; ensure methods that previously assumed activity non-null
handle a null gracefully.
---
Nitpick comments:
In
`@android/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.java`:
- Around line 94-126: scheduleAllActiveJobs currently spins a raw Thread and can
return silently (e.g., when CenterMachineDto is null), giving callers like
MainActivity.initializeAppComponent no visibility; change scheduleAllActiveJobs
to accept a callback or return a CompletableFuture (e.g., a
CompletableFuture<Void> or CompletableFuture<Boolean>) so callers receive
success/failure/skip status, resolve the future (or invoke the callback) on
normal completion with the number of scheduled jobs or a skipped flag, and
complete exceptionally (or pass an error) on exceptions; update usages such as
MainActivity.initializeAppComponent to await or register the callback and add
structured Log.i/Log.w/Log.e messages in scheduleAllActiveJobs (and around
scheduleJob calls) to provide clear initialization telemetry.
- Around line 139-154: The getExcludedJobIds and addJobIdsFromString methods in
SyncScheduler duplicate logic found in MasterDataSyncApi; extract them into a
shared utility (e.g., JobExclusionHelper) with public static methods
getExcludedJobIds(GlobalParamRepository) and addJobIdsFromString(Set<String>,
String), then replace SyncScheduler.getExcludedJobIds and MasterDataSyncApi's
copies to call JobExclusionHelper.getExcludedJobIds(globalParamRepository) (and
JobExclusionHelper.addJobIdsFromString where needed) to remove duplication and
centralize behavior while keeping the same method semantics and use of
RegistrationConstants.COMMA.
- Around line 58-66: Rename the misleading BatchJob.getIntervalMillis(...) to
BatchJob.getNextExecutionTimeMillis(...) (it currently returns an absolute
next-execution timestamp from
CronParserUtil.getNextExecutionTimeInMillis(cronExp)), update its declaration in
BatchJob.java and all call sites (e.g., SyncScheduler.scheduleJob where
batchJob.getIntervalMillis(jobApiName) is used) so callers compute delay as
nextExecutionTime - System.currentTimeMillis(), and keep the existing
behavior/logging unchanged; ensure no remaining references to getIntervalMillis
remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf57f1e6-5482-46d1-9cba-eb1a20687f95
📒 Files selected for processing (13)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/io/mosip/registration_client/AppComponent.javaandroid/app/src/main/java/io/mosip/registration_client/HostApiModule.javaandroid/app/src/main/java/io/mosip/registration_client/MainActivity.javaandroid/app/src/main/java/io/mosip/registration_client/RegistrationClientApp.javaandroid/app/src/main/java/io/mosip/registration_client/SyncWorker.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/AuthenticationApi.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/app/src/main/java/io/mosip/registration_client/utils/BatchJob.javaandroid/app/src/main/java/io/mosip/registration_client/utils/CustomToast.javaandroid/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/NetworkModule.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java
android/app/src/main/java/io/mosip/registration_client/api_services/AuthenticationApi.java
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java
Outdated
Show resolved
Hide resolved
MadhuMosip
changed the title
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java (1)
77-79: Consider logging the exception instead of silently ignoring it.The
catch (Exception ignored)block suppresses all errors during token restoration, which could mask database access failures, malformed tokens, or other unexpected issues. While returningnullis a safe fallback, adding a log statement would aid debugging in production.♻️ Proposed fix to add logging
- } catch (Exception ignored) { + } catch (Exception e) { + android.util.Log.w("RestAuthInterceptor", "Token restoration failed", e); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java` around lines 77 - 79, The catch block in RestAuthInterceptor (the catch (Exception ignored) that returns null during token restoration) should log the caught exception before returning null; update that catch to call the class logger (e.g., LOGGER.error(...) or Android Log.e(...)) with a short context message like "failed to restore auth token" and include the exception so stacktrace/details are preserved, then continue returning null for the safe fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/io/mosip/registration_client/SyncWorker.java`:
- Around line 54-66: The code currently catches all exceptions around
CountDownLatch.await(...) in SyncWorker which hides InterruptedException and
clears the thread's interrupted status; add a dedicated catch
(InterruptedException ie) right after the try block around latch.await, call
Thread.currentThread().interrupt() to restore the flag, log the interruption
(including jobApiName) and return Result.retry() (or the appropriate WorkManager
result your flow expects), leaving the existing generic Exception catch to
handle other failures.
- Line 27: The SYNC_TIMEOUT_MINUTES constant is set to WorkManager's 10-minute
hard limit which leaves no headroom to finish cleanup and return Result; update
the SyncWorker implementation by either lowering SYNC_TIMEOUT_MINUTES to a value
under 10 minutes (e.g., 9) so the timeout branch and result return have time to
execute, or refactor SyncWorker to a callback-driven worker (convert the class
from Worker to ListenableWorker or CoroutineWorker and implement asynchronous
completion callbacks) and remove the blocking timeout logic; reference
SYNC_TIMEOUT_MINUTES and the SyncWorker class when making the change.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java`:
- Around line 24-28: Test instantiation of the RestAuthInterceptor was not
updated after the constructor changed to accept a UserTokenDao; add a `@Mock`
field for UserTokenDao (e.g., mockUserTokenDao) in the test class and update the
interceptor construction to call new RestAuthInterceptor(mockContext,
mockUserTokenDao) so the test uses the current constructor signature; ensure the
mock is initialized (with `@RunWith`(MockitoJUnitRunner.class) or
MockitoAnnotations.initMocks) and replace any old new
RestAuthInterceptor(mockContext) usages accordingly.
- Around line 82-92: The JWT expiration leeway in
RestAuthInterceptor.isTokenValid is too small (jwt.isExpired(15)); increase the
leeway to a more appropriate value (e.g., 900 for 15 minutes) or make it
configurable, update the jwt.isExpired(...) call to use the new value, and
ensure the change is applied inside the isTokenValid(String token) method that
constructs the JWT and checks expiration.
---
Nitpick comments:
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java`:
- Around line 77-79: The catch block in RestAuthInterceptor (the catch
(Exception ignored) that returns null during token restoration) should log the
caught exception before returning null; update that catch to call the class
logger (e.g., LOGGER.error(...) or Android Log.e(...)) with a short context
message like "failed to restore auth token" and include the exception so
stacktrace/details are preserved, then continue returning null for the safe
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4779b57b-1354-402a-89c2-8e7377147337
📒 Files selected for processing (3)
android/app/src/main/java/io/mosip/registration_client/SyncWorker.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.javaandroid/packetmanager/src/main/java/io/mosip/registration/packetmanager/util/StorageUtils.java
android/app/src/main/java/io/mosip/registration_client/SyncWorker.java
Outdated
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java
Show resolved
Hide resolved
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
There was a problem hiding this comment.
Actionable comments posted: 2
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/test/java/io/mosip/registration/clientmanager/service/RestAuthInterceptorTest.java (1)
55-70:⚠️ Potential issue | 🔴 CriticalTest will fail:
"token123"is not a valid JWT and will be rejected byisTokenValid().The
RestAuthInterceptorvalidates tokens usingJWT.isExpired(). The test returns"token123"fromfetchAuthToken(), which cannot be parsed as a JWT (valid format isheader.payload.signature). TheisTokenValid()method attempts to parse it as a JWT and catches the exception, returningfalse. This causesensureValidSessionToken()to attempt DB restoration viauserTokenDao.findByUsername(), which is not mocked in this test. The restoration fails, returningnull, so the header is never added—causing the test to fail.You need to either:
- Use a valid (non-expired) JWT in the test, or
- Mock the restoration path by configuring
mockUserTokenDao.findByUsername()and SharedPreferences behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/RestAuthInterceptorTest.java` around lines 55 - 70, The failing test returns a non-JWT string from mockSessionManager.fetchAuthToken(), causing RestAuthInterceptor.isTokenValid() to reject it and trigger ensureValidSessionToken() which calls mockUserTokenDao.findByUsername() (not stubbed); fix by either returning a well-formed, unexpired JWT from mockSessionManager.fetchAuthToken() so isTokenValid() passes, or stub the restoration path: mockUserTokenDao.findByUsername(...) to return a valid UserToken and mock the SharedPreferences/session writes used by ensureValidSessionToken(), ensuring the interceptor adds the Cookie header and proceeds; update the test setup around RestAuthInterceptor, mockSessionManager.fetchAuthToken(), isTokenValid()/ensureValidSessionToken() interactions accordingly.
🧹 Nitpick comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java (1)
79-80: Consider logging the exception for debugging purposes.Silently swallowing exceptions makes it difficult to diagnose token restoration failures in production. Adding a log statement would help with troubleshooting.
💡 Suggested improvement
- } catch (Exception ignored) { + } catch (Exception e) { + android.util.Log.w("RestAuthInterceptor", "Token restoration failed", e); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java` around lines 79 - 80, The catch block in RestAuthInterceptor (the exception handler that currently does "catch (Exception ignored) { return null; }") swallows errors; update it to log the caught Exception before returning null so token restoration failures are visible in logs—use the class logger (e.g., Log.e or the existing logger instance in RestAuthInterceptor) and include the exception message and stack trace in the log call while keeping the current null return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/io/mosip/registration_client/SyncWorker.java`:
- Around line 54-66: The current worker only waits for executeJobByApiName to
finish (using CountDownLatch) but ignores the boolean success passed to the
completion callback; update the callback handling in SyncWorker to capture the
boolean success (e.g., AtomicBoolean or similar) when
syncApi.executeJobByApiName invokes SyncCompletionCallback.onComplete(boolean
success), await the latch, and then: if success is false return Result.retry()
(and do not call scheduler.scheduleJob), otherwise schedule the next run via
scheduler.scheduleJob(...) and return Result.success(); ensure you reference
syncApi.executeJobByApiName(...) and SyncCompletionCallback.onComplete(boolean)
when making the change.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java`:
- Around line 43-82: ensureValidSessionToken risks a race because
sessionManager.saveAuthToken(dbToken) uses async apply(); change to a
synchronous write to guarantee other threads reading SharedPreferences after the
lock see the token. Add a new method in SessionManager (e.g.,
saveAuthTokenSync(String token) that uses SharedPreferences.Editor.commit()) and
call that from RestAuthInterceptor.ensureValidSessionToken instead of
saveAuthToken; keep returning dbToken as before so callers can use the returned
value immediately.
---
Outside diff comments:
In
`@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/RestAuthInterceptorTest.java`:
- Around line 55-70: The failing test returns a non-JWT string from
mockSessionManager.fetchAuthToken(), causing RestAuthInterceptor.isTokenValid()
to reject it and trigger ensureValidSessionToken() which calls
mockUserTokenDao.findByUsername() (not stubbed); fix by either returning a
well-formed, unexpired JWT from mockSessionManager.fetchAuthToken() so
isTokenValid() passes, or stub the restoration path:
mockUserTokenDao.findByUsername(...) to return a valid UserToken and mock the
SharedPreferences/session writes used by ensureValidSessionToken(), ensuring the
interceptor adds the Cookie header and proceeds; update the test setup around
RestAuthInterceptor, mockSessionManager.fetchAuthToken(),
isTokenValid()/ensureValidSessionToken() interactions accordingly.
---
Nitpick comments:
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java`:
- Around line 79-80: The catch block in RestAuthInterceptor (the exception
handler that currently does "catch (Exception ignored) { return null; }")
swallows errors; update it to log the caught Exception before returning null so
token restoration failures are visible in logs—use the class logger (e.g., Log.e
or the existing logger instance in RestAuthInterceptor) and include the
exception message and stack trace in the log call while keeping the current null
return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cfa6ff99-88a3-4e78-9472-626a48f25a2a
📒 Files selected for processing (3)
android/app/src/main/java/io/mosip/registration_client/SyncWorker.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/RestAuthInterceptorTest.java
...nager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java
Show resolved
Hide resolved
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (2)
596-737:⚠️ Potential issue | 🟠 MajorMake async completion reporting exception-safe.
The outer
try/catchonly covers the thread body, not the later callbacks. IflogLastSyncCompletionDateTime(...)oronResponseComplete()throws inside one of these async branches,onSyncJobComplete(...)andnotifyComplete(...)are skipped, sorunningSyncJobsstays inflated and any worker waiting ononCompletecan hang forever. Please wrap each callback body intry/finallyor a shared helper so completion is always signalled exactly once.🛠️ Safer callback pattern
- batchJob.uploadRegistrationPackets(context, () -> { - Log.d(getClass().getSimpleName(), "Registration packet upload job completed"); - masterDataService.logLastSyncCompletionDateTime(jobId); - onSyncJobComplete(jobId, true, false); - notifyComplete(onComplete, true); - }); + batchJob.uploadRegistrationPackets(context, () -> { + boolean success = false; + try { + Log.d(getClass().getSimpleName(), "Registration packet upload job completed"); + masterDataService.logLastSyncCompletionDateTime(jobId); + success = true; + } catch (Exception e) { + Log.e(TAG, "registrationPacketUploadJob completion failed", e); + } finally { + onSyncJobComplete(jobId, success, false); + notifyComplete(onComplete, success); + } + });Apply the same pattern to the other async callback branches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java` around lines 596 - 737, The async callback bodies in executeJobByApiName (e.g., the lambdas passed to batchJob.uploadRegistrationPackets, batchJob.syncRegistrationPackets, masterDataService.syncMasterData/syncGlobalParamsData/syncUserDetails/syncCertificate/syncCACertificates, preRegistrationDataSyncService.fetchPreRegistrationIds, and the direct branches like packetService.syncAllPacketStatus) must guarantee final completion signaling; wrap each callback body in a try/finally (or call a shared helper) so that regardless of exceptions from masterDataService.logLastSyncCompletionDateTime or masterDataService.onResponseComplete (or any other thrown error) you always invoke onSyncJobComplete(jobId, successFlag, false) and notifyComplete(onComplete, successFlag) exactly once; ensure the helper/ finally computes success safely (treat exceptions as failure) and still logs errors (Log.e) before signalling completion.
558-564:⚠️ Potential issue | 🔴 CriticalRemove duplicate job scheduling in
modifyJobCronExpression().This method calls
jobManagerService.refreshJobStatus(jobDef)which internally schedules the job via Android's JobScheduler (scheduleJob(jobId, jobDef.getApiName(), syncFreq)), then immediately callssyncScheduler.scheduleJob(context, jobDef.getApiName())to schedule via WorkManager. Changing a cron expression triggers both schedulers, creating two concurrent sync jobs for the same API endpoint. Either cancel the legacy JobScheduler job before the WorkManager call, or consolidate on the new scheduler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java` around lines 558 - 564, modifyJobCronExpression currently calls jobManagerService.refreshJobStatus(jobDef) which reschedules the legacy JobScheduler and then immediately calls syncScheduler.scheduleJob(context, jobDef.getApiName()), causing duplicate jobs; either stop the legacy JobScheduler before scheduling WorkManager or remove the WorkManager call. Fix by invoking the existing JobScheduler cancel method (e.g., jobManagerService.cancelJob or jobManagerService.unscheduleJob with jobDef.getJobId()/getApiName) immediately after refreshJobStatus and before syncScheduler.scheduleJob, or remove the syncScheduler.scheduleJob(context, jobDef.getApiName()) call and rely solely on refreshJobStatus; ensure you update modifyJobCronExpression to use one scheduler only and reference jobManagerService.refreshJobStatus, jobManagerService.cancelJob/unscheduleJob, syncScheduler.scheduleJob and modifyJobCronExpression when making the change.
🧹 Nitpick comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java (1)
79-80: Don't drop restore failures without any signal.This catch now swallows JWT parsing/role-validation/persistence failures and turns them into a bare
null, so background sync just loses its auth header with nothing useful in logs.Possible improvement
- } catch (Exception ignored) { + } catch (Exception e) { + Log.w(TAG, "Failed to restore session token", e); return null; }Add the missing logger members as well:
import android.util.Log; private static final String TAG = RestAuthInterceptor.class.getSimpleName();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java` around lines 79 - 80, The current catch in RestAuthInterceptor swallows exceptions (catch(Exception ignored) { return null; }) which hides JWT/role/persistence failures; replace it to log the exception before returning null: add import android.util.Log and a TAG constant (e.g., private static final String TAG = RestAuthInterceptor.class.getSimpleName()), then change the catch to catch(Exception e) { Log.e(TAG, "Failed to restore auth token/roles", e); return null; } so failures are recorded while retaining the existing return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.java`:
- Around line 107-111: The current scheduling call in SyncScheduler uses
ExistingWorkPolicy.REPLACE which cancels pending constrained work; in the method
that bootstraps jobs (invoked by
scheduleAllActiveJobs()/configureFlutterEngine()), change the enqueueUniqueWork
call that builds uniqueWorkName ("sync_"+jobApiName) to use
ExistingWorkPolicy.KEEP instead of REPLACE so existing enqueued work waiting on
constraints is preserved; reserve REPLACE only for explicit reschedule/edit
flows. Ensure the change is applied to the
WorkManager.getInstance(context).enqueueUniqueWork(...) invocation that creates
workRequest for sync_{api}.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/SessionManager.java`:
- Around line 93-94: The method saveAuthTokenSync currently calls
editor.commit() but ignores its boolean result and still returns roles; update
saveAuthTokenSync to capture the commit result (boolean success =
editor.commit()) and if it is false, fail the method by returning null (or an
error indicator used elsewhere) instead of returning roles; also add a log/error
via SessionManager's logger to record commit failure so callers/interceptor
won't assume the session was persisted when commit() failed.
- Around line 106-110: SessionManager: guard against null realmAccess and
null/empty roles before dereferencing the list returned from
jwt.getClaim(REALM_ACCESS).asObject(Map.class); if jwt.getClaim(REALM_ACCESS) is
missing or asObject(...) returns null, or the "roles" entry is null/not a List
or empty, throw the same authorization Exception ("Unauthorized access, No
roles"). Locate the code around jwt.getClaim(REALM_ACCESS).asObject(Map.class)
and the List<String> roles assignment and add explicit null checks (or type
checks) for realmAccess and roles before calling roles.isEmpty(), ensuring a
consistent exception path for malformed tokens.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java`:
- Around line 19-20: The JWT expiry leeway is inconsistent: RestAuthInterceptor
defines TOKEN_EXPIRY_LEEWAY_SECONDS = 900 (15 minutes) while SessionManager
calls jwt.isExpired(15) (15 seconds); standardize the leeway value and intent
across both locations by choosing a single clock-skew leeway (e.g.,
LEAWAY_SECONDS) and using it for all jwt.isExpired(...) checks (update
RestAuthInterceptor.TOKEN_EXPIRY_LEEWAY_SECONDS and replace the literal 15 in
SessionManager), and if you need an extra safety buffer for network delays
implement a separate safety-check variable (e.g., SAFETY_BUFFER_SECONDS) applied
only at decision points rather than conflating it with the leeway.
---
Outside diff comments:
In
`@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java`:
- Around line 596-737: The async callback bodies in executeJobByApiName (e.g.,
the lambdas passed to batchJob.uploadRegistrationPackets,
batchJob.syncRegistrationPackets,
masterDataService.syncMasterData/syncGlobalParamsData/syncUserDetails/syncCertificate/syncCACertificates,
preRegistrationDataSyncService.fetchPreRegistrationIds, and the direct branches
like packetService.syncAllPacketStatus) must guarantee final completion
signaling; wrap each callback body in a try/finally (or call a shared helper) so
that regardless of exceptions from
masterDataService.logLastSyncCompletionDateTime or
masterDataService.onResponseComplete (or any other thrown error) you always
invoke onSyncJobComplete(jobId, successFlag, false) and
notifyComplete(onComplete, successFlag) exactly once; ensure the helper/ finally
computes success safely (treat exceptions as failure) and still logs errors
(Log.e) before signalling completion.
- Around line 558-564: modifyJobCronExpression currently calls
jobManagerService.refreshJobStatus(jobDef) which reschedules the legacy
JobScheduler and then immediately calls syncScheduler.scheduleJob(context,
jobDef.getApiName()), causing duplicate jobs; either stop the legacy
JobScheduler before scheduling WorkManager or remove the WorkManager call. Fix
by invoking the existing JobScheduler cancel method (e.g.,
jobManagerService.cancelJob or jobManagerService.unscheduleJob with
jobDef.getJobId()/getApiName) immediately after refreshJobStatus and before
syncScheduler.scheduleJob, or remove the syncScheduler.scheduleJob(context,
jobDef.getApiName()) call and rely solely on refreshJobStatus; ensure you update
modifyJobCronExpression to use one scheduler only and reference
jobManagerService.refreshJobStatus, jobManagerService.cancelJob/unscheduleJob,
syncScheduler.scheduleJob and modifyJobCronExpression when making the change.
---
Nitpick comments:
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java`:
- Around line 79-80: The current catch in RestAuthInterceptor swallows
exceptions (catch(Exception ignored) { return null; }) which hides
JWT/role/persistence failures; replace it to log the exception before returning
null: add import android.util.Log and a TAG constant (e.g., private static final
String TAG = RestAuthInterceptor.class.getSimpleName()), then change the catch
to catch(Exception e) { Log.e(TAG, "Failed to restore auth token/roles", e);
return null; } so failures are recorded while retaining the existing return
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa4ebaa0-9057-49f6-8788-77bfc1cc8b7a
📒 Files selected for processing (7)
android/app/src/main/java/io/mosip/registration_client/MainActivity.javaandroid/app/src/main/java/io/mosip/registration_client/RegistrationClientApp.javaandroid/app/src/main/java/io/mosip/registration_client/SyncWorker.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/SessionManager.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/io/mosip/registration_client/SyncWorker.java
android/app/src/main/java/io/mosip/registration_client/utils/SyncScheduler.java
Outdated
Show resolved
Hide resolved
...d/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/SessionManager.java
Outdated
Show resolved
Hide resolved
...d/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/SessionManager.java
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/clientmanager/interceptor/RestAuthInterceptor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
ase-101
reviewed
Mar 11, 2026
| @@ -221,31 +222,14 @@ public void logout(@NonNull AuthResponsePigeon.Result<String> result) { | |||
| @Override | |||
| public void stopAlarmService(@NonNull AuthResponsePigeon.Result<String> result) { | |||
Collaborator
There was a problem hiding this comment.
in which cases do we use stopAlarmService?
ase-101
reviewed
Mar 11, 2026
| } | ||
|
|
||
| // Repopulate session prefs so subsequent calls use the same token. | ||
| sessionManager.saveAuthTokenSync(dbToken); |
Collaborator
There was a problem hiding this comment.
During the background task, if we are not able to find the session to get the auth-token. We should not create dummy session.
For background tasks, if session is not available, directly hit the user token table to identify latest valid auth-token and use it without creating the session.
…on token is expired Signed-off-by: Madhuravas reddy <madhu@mosip.io>
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.
Summary by CodeRabbit