RCF-1278: Editable Cron Job in Scheduled Job Settings #663
Conversation
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
WalkthroughAdds platform-channel APIs and UI to view/validate/modify per-job cron expressions; wires LocalConfigService through Android DI and clientmanager layers, persisting local overrides, emitting reschedule broadcasts, and introducing Flutter polling/state to surface job statuses and permit inline cron editing. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Scheduled Jobs UI
participant Provider as SyncProvider
participant DartSvc as SyncResponseService (Dart)
participant Pigeon as SyncApi (Pigeon)
participant AndroidApi as MasterDataSyncApi
participant Config as LocalConfigService
participant JobMgr as JobManagerService
User->>UI: Edit cron expression
UI->>DartSvc: isValidCronExpression(cron)
DartSvc->>Pigeon: isValidCronExpression
Pigeon->>AndroidApi: isValidCronExpression
AndroidApi->>AndroidApi: validate via CronExpressionParser
AndroidApi-->>Pigeon: bool
Pigeon-->>DartSvc: bool
DartSvc-->>UI: validation result
alt valid
UI->>DartSvc: modifyJobCronExpression(jobId, cron)
DartSvc->>Pigeon: modifyJobCronExpression
Pigeon->>AndroidApi: modifyJobCronExpression
AndroidApi->>Config: modifyJob(jobId, cron)
Config->>Config: validate & persist
AndroidApi->>JobMgr: refreshJobStatus(jobId)
JobMgr->>Config: getSyncFrequency(job)
Config-->>JobMgr: override cron if present
AndroidApi-->>Pigeon: success
Pigeon-->>DartSvc: success
DartSvc-->>UI: success -> trigger app restart
AndroidApi->>AndroidApi: send RESCHEDULE_JOB broadcast
AndroidApi->>JobMgr: createBackgroundTask(job)
else invalid
UI->>UI: show error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
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
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: 4
Fix all issues with AI Agents 🤖
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java:
- Around line 93-113: modifyJob performs a soft-delete via
updateLocalPreference(LocalPreferences, ...) and then creates a new record via
saveLocalPreference(name, value, ...), but these are separate DB operations and
must be atomic; wrap both calls in a single transaction by annotating modifyJob
with @Transaction (or call database.runInTransaction { ... } around
updateLocalPreference and saveLocalPreference) to ensure both succeed or both
roll back, preventing inconsistent state or race conditions when
LocalPreferences is concurrently modified.
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java:
- Around line 43-46: The service method LocalConfigServiceImpl.modifyJob must
validate the cron expression before delegating to the DAO: inject
CronExpressionParser into LocalConfigServiceImpl, call it to validate the
incoming value and throw a clear exception (e.g., IllegalArgumentException or a
domain-specific ValidationException) when invalid, then call
localConfigDAO.modifyJob only for valid expressions. Also make
LocalConfigDAOImpl.modifyJob atomic to avoid race conditions by either marking
the method synchronized or, preferably, wrapping the read-check-write in a
database transaction with an appropriate lock (SELECT ... FOR UPDATE or
equivalent) so the read/validate/update sequence is executed atomically.
In @lib/ui/settings/widgets/scheduled_jobs_settings.dart:
- Around line 212-216: The code calls
_syncResponseService.modifyJobCronExpression with widget.job.id without
validating it; add a guard that checks widget.job.id for null or empty (e.g., if
(widget.job.id == null || widget.job.id!.isEmpty) ) before invoking
modifyJobCronExpression, and handle the case by returning early and surfacing an
error/notification (or logging) so modifyJobCronExpression is never called with
an empty id; update any surrounding try/catch flow to reflect the early return.
🧹 Nitpick comments (6)
lib/platform_spi/sync_response_service.dart (1)
35-37: LGTM! Consider error details beyond boolean returns.The method signatures are appropriate for the cron job management feature. The
modifyJobCronExpressionmethod returns a boolean to indicate success/failure, which is simple and clear.For enhanced error reporting in the UI, you might consider returning a result object that includes error messages or codes, but the current boolean approach is acceptable for a "Chill" review.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAO.java (1)
36-41: Rename parameternametojobIdfor semantic clarity and document the exception-throwing behavior.Parameter naming is inconsistent with the caller in
MasterDataSyncApi.java:464, which passesjobIdandcronExpressionto a method that declares parameters asnameandvalue. Renaming tojobIdandcronExpressionwould improve code readability and consistency across the codebase.Additionally, the implementation (in
LocalConfigDAOImpl.java:109-111) throws exceptions on database errors, but the interface (LocalConfigDAO.java) does not document this with a@throwsclause. Add@throws Exceptionto the method documentation to make the error handling contract explicit to callers.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
80-91: Consider surfacing errors for critical configuration reads.The
getValuemethod catches and logs all exceptions but returnsnull, making it difficult for callers to distinguish between "configuration not found" and "database error occurred."If configuration retrieval is critical for job execution, consider:
- Rethrowing exceptions for database errors while still returning
nullfor "not found" cases- Or documenting that
nullmay indicate either scenario and callers should check logsThis is a minor point and depends on your error handling philosophy—current implementation may be intentional for fail-safe behavior.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/JobManagerServiceImpl.java (1)
88-88: Consider usinggetSyncFrequencyfor consistency.Line 88 still passes
jobDef.getSyncFreq()directly toscheduleJob, while Line 173 now uses the newgetSyncFrequency(jobDef)helper.Although
scheduleJobdoesn't currently use thesyncFreqparameter for actual scheduling (it uses hardcodedJOB_PERIODIC_SECONDS), usinggetSyncFrequency(jobDef)here would improve consistency across the codebase and make future refactoring easier if the scheduling logic changes to honor the sync frequency.🔎 Proposed change for consistency
- if (!isJobScheduled(jobId)) - scheduleJob(jobId, jobDef.getApiName(), jobDef.getSyncFreq()); + if (!isJobScheduled(jobId)) + scheduleJob(jobId, jobDef.getApiName(), getSyncFrequency(jobDef));android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (1)
461-470: Consider validating cron expression before persisting.The method does not validate the cron expression before modifying the job. While the UI calls
isValidCronExpressionfirst, adding server-side validation here provides defense in depth against invalid data being persisted.🔎 Proposed fix to add validation
@Override public void modifyJobCronExpression(@NonNull String jobId, @NonNull String cronExpression, @NonNull MasterDataSyncPigeon.Result<Boolean> result) { try { + if (!CronExpressionParser.isValidCronExpression(cronExpression)) { + result.success(false); + return; + } localConfigService.modifyJob(jobId, cronExpression); result.success(true); } catch (Exception e) { Log.e(TAG, "Failed to modify job cron expression", e); result.error(e); } }lib/ui/settings/widgets/scheduled_jobs_settings.dart (1)
38-52: Consider consolidatingSyncResponseServiceinstantiation.Multiple
SyncResponseServiceinstances are created: in_loadPermittedJobs(line 40), as a field in_JobCardState(line 131), and inside_triggerJobSync(line 244). Consider using a single instance for consistency and to reduce object allocation overhead.🔎 Proposed consolidation for _ScheduledJobsSettingsState
class _ScheduledJobsSettingsState extends State<ScheduledJobsSettings> { List<String?> _permittedJobs = []; bool _isLoadingPermittedJobs = true; + final SyncResponseService _syncResponseService = SyncResponseService(); @override void initState() { super.initState(); _loadPermittedJobs(); } Future<void> _loadPermittedJobs() async { try { - final service = SyncResponseService(); - final permittedJobs = await service.getPermittedJobs(); + final permittedJobs = await _syncResponseService.getPermittedJobs();Also applies to: 130-131
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
android/app/src/main/java/io/mosip/registration_client/HostApiModule.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/AppModule.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAO.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/JobManagerServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/LocalConfigService.javalib/platform_android/sync_response_service_impl.dartlib/platform_spi/sync_response_service.dartlib/ui/settings/widgets/scheduled_jobs_settings.dartpigeon/master_data_sync.dart
🧰 Additional context used
🧬 Code graph analysis (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
RegistrationConstants(9-139)
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/util/CronExpressionParser.java (1)
CronExpressionParser(19-65)
🔇 Additional comments (15)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAO.java (1)
29-34: LGTM!The method signature and documentation are clear. The nullable return value is properly documented.
android/app/src/main/java/io/mosip/registration_client/HostApiModule.java (1)
187-214: LGTM!The dependency injection wiring correctly adds
LocalConfigServiceas a dependency toMasterDataSyncApi. The pattern is consistent with other provider methods in this module.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java (2)
38-41: LGTM!Clean delegation to the DAO layer. The method signature matches the interface contract.
48-51: LGTM!Proper use of the
PERMITTED_JOB_TYPEconstant fromRegistrationConstantsensures consistency with the permitted configuration type system.pigeon/master_data_sync.dart (1)
66-71: All three async API methods are properly implemented in MasterDataSyncApi.java.Verification confirms that the Android implementation includes all three methods with correct signatures and proper async handling via the Pigeon Result callback pattern:
getPermittedJobs()— delegates tolocalConfigService.getPermittedJobs()isValidCronExpression(String cronExpression)— validates usingCronExpressionParsermodifyJobCronExpression(String jobId, String cronExpression)— delegates tolocalConfigService.modifyJob()Each method includes appropriate exception handling and logging.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/LocalConfigService.java (1)
26-44: LGTM! Well-documented interface additions.The three new methods are clearly documented and follow consistent patterns with the existing interface:
getValue(String name): Clean read operationmodifyJob(String name, String value): Clear write operation for job configurationgetPermittedJobs(): Straightforward query for permitted job IDsThe method signatures and documentation are appropriate for the feature requirements.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/AppModule.java (1)
263-264: LGTM! Proper dependency injection setup.The LocalConfigService dependency is correctly wired into JobManagerServiceImpl through Dagger. The singleton scope is maintained, and the parameter ordering is consistent with the constructor signature.
lib/platform_android/sync_response_service_impl.dart (1)
275-315: LGTM! Consistent error handling and implementation.The three new platform bridge methods follow the established error handling patterns in this file:
- Proper
PlatformExceptionhandling with debug logging- Safe fallback values on error (empty list for
getPermittedJobs,falsefor boolean methods)- Consistent structure with existing methods
The implementation correctly delegates to the platform API layer.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/JobManagerServiceImpl.java (1)
191-204: LGTM! Robust override mechanism with proper fallback.The
getSyncFrequencyhelper correctly implements the local configuration override logic:
- Null-safe check for
localConfigServiceensures graceful degradation- Trim validation prevents whitespace-only values from being used
- Clean fallback to default
syncJob.getSyncFreq()when no override existsThis enables per-job cron expression customization while maintaining backward compatibility.
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (3)
62-67: LGTM! Dependency injection for LocalConfigService is properly wired.The new imports, field declaration, constructor parameter, and assignment for
LocalConfigServicefollow the established pattern used for other services in this class.Also applies to: 105-105, 126-126, 152-152
439-448: LGTM! Proper null handling and error propagation.The method correctly handles null results by returning an empty list, and exception handling follows the established pattern in this class.
450-459: LGTM! Defensive error handling for cron validation.The method correctly returns
falseon any exception rather than propagating an error, which is appropriate for a validation method. The outer try-catch is redundant givenCronExpressionParser.isValidCronExpressionalready handles exceptions internally, but it provides an extra safety net.lib/ui/settings/widgets/scheduled_jobs_settings.dart (3)
14-26: LGTM! Appropriate conversion to StatefulWidget.The widget correctly extends
StatefulWidgetto manage the permitted jobs list and loading state. ThecreateState()implementation follows Flutter conventions.
145-149: LGTM! Proper controller lifecycle management.The
TextEditingControlleris correctly initialized ininitStateand disposed indispose, preventing memory leaks.
363-382: LGTM! Conditional UI for cron editing is well implemented.The edit button only appears for permitted jobs, and the sync button is correctly hidden during editing mode to prevent conflicting user actions.
Also applies to: 386-386
...anager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
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/service/JobManagerServiceImpl.java (1)
99-116: ThesyncFreqparameter is not used in scheduling logic.The
scheduleJobmethod accepts asyncFreqparameter but ignores it entirely. The job is always scheduled with a fixedperiodMillis = JOB_PERIODIC_SECONDS * 1000L(15 minutes). This means custom cron expressions fromLocalConfigServicewon't actually affect the JobScheduler timing—only the "next sync time" display will reflect the custom cron.If the intent is to honor custom cron expressions for scheduling, the
syncFreqshould influenceperiodMillisor use a different scheduling strategy.🔎 Example fix to use cron-derived interval
@Override public int scheduleJob(int jobId, String apiName, String syncFreq) { Class<?> clientJobService = getJobServiceImplClass(apiName); if (clientJobService == null) { throw new NotImplementedException("Job service : " + apiName + " not implemented"); } ComponentName componentName = new ComponentName(context, clientJobService); JobInfo info; - //To schedule periodically - long periodMillis = JOB_PERIODIC_SECONDS * 1000L; + // Calculate period from cron expression if valid, otherwise use default + long periodMillis = JOB_PERIODIC_SECONDS * 1000L; + if (CronExpressionParser.isValidCronExpression(syncFreq)) { + Instant nextExecution = CronExpressionParser.getNextExecutionTime(syncFreq); + if (nextExecution != null) { + long delay = nextExecution.toEpochMilli() - System.currentTimeMillis(); + if (delay > 0) { + periodMillis = delay; + } + } + } info = new JobInfo.Builder(jobId, componentName) .setRequiresCharging(false) .setPersisted(true) .setPeriodic(periodMillis) .build(); return jobScheduler.schedule(info); }
♻️ Duplicate comments (1)
lib/ui/settings/widgets/scheduled_jobs_settings.dart (1)
226-230: Guard against null/empty job ID before calling modify.As noted in a previous review, if
widget.job.idis null or empty, an empty string is passed tomodifyJobCronExpression, which could result in undefined behavior or a failed operation without clear feedback.
🧹 Nitpick comments (4)
android/app/src/main/java/io/mosip/registration_client/MainActivity.java (2)
282-286: Consider catching a specific exception type or revising the comment.The try-catch block provides defensive unregistration, but it catches all exceptions which might hide unexpected issues. Since
rescheduleReceiveris always registered inonCreate(line 237), it should always be safe to unregister.Suggested improvements
Option 1: Catch specific exception
try { unregisterReceiver(rescheduleReceiver); -} catch (Exception e) { - // Receiver might not be registered, ignore +} catch (IllegalArgumentException e) { + // Receiver was not registered (edge case), ignore + Log.d(getClass().getSimpleName(), "rescheduleReceiver was not registered", e); }Option 2: Remove try-catch for consistency (if confident it's always registered)
unregisterReceiver(broadcastReceiver); -try { - unregisterReceiver(rescheduleReceiver); -} catch (Exception e) { - // Receiver might not be registered, ignore -} +unregisterReceiver(rescheduleReceiver);
217-226: The implementation is correct – the RESCHEDULE_JOB broadcast is confirmed to be sent from MasterDataSyncApi when a cron expression is modified.The rescheduleReceiver correctly handles the broadcast and triggers job rescheduling. The flow is working as intended: when cron expressions change, MasterDataSyncApi sends the broadcast, MainActivity receives it, and createBackgroundTask is called to reschedule AlarmManager-based jobs.
Consider using package-prefixed action constants for better maintainability and to avoid potential conflicts with other apps. Define broadcast actions as public static final constants (e.g.,
io.mosip.registration_client.RESCHEDULE_JOB) rather than plain string literals. This would also align with the existing pattern if similar constants are defined elsewhere in the codebase.lib/ui/settings/widgets/scheduled_jobs_settings.dart (2)
145-157: MissingsetStatecall after async cron expression load.
_loadCronExpressionupdates_cronController.textasynchronously but doesn't callsetState. WhileTextEditingControllerupdates will reflect in the TextField, this pattern is inconsistent with other async loaders in this class (_loadLastSyncTime,_loadNextSyncTime) that do callsetState.If there's any UI state dependent on whether the cron was loaded, or if you want to ensure a rebuild after load, consider wrapping the assignment in
setStateor at minimum callingsetState(() {})after updating the controller.🔎 Optional fix for consistency
Future<void> _loadCronExpression() async { if (widget.job.id != null && widget.job.id!.isNotEmpty) { // Check for custom cron expression first (matches desktop client logic) final customCron = await _syncResponseService.getValue(widget.job.id!); - if (customCron != null && customCron.trim().isNotEmpty) { - _cronController.text = customCron; // Use saved custom cron expression - } else { - _cronController.text = widget.job.syncFreq ?? ''; // Use default from DB - } + setState(() { + if (customCron != null && customCron.trim().isNotEmpty) { + _cronController.text = customCron; // Use saved custom cron expression + } else { + _cronController.text = widget.job.syncFreq ?? ''; // Use default from DB + } + }); } else { - _cronController.text = widget.job.syncFreq ?? ''; + setState(() { + _cronController.text = widget.job.syncFreq ?? ''; + }); } }
246-252: Consider implications of forced app restart.
Restart.restartApp()forcefully restarts the entire application. This could lead to:
- Loss of any unsaved work in other parts of the app
- Poor user experience if users are in the middle of another task
Consider alternative approaches such as:
- Refreshing only the job scheduling components
- Prompting the user before restart
- Deferring restart until the user navigates away or closes the app
🔎 Proposed improvement: confirm before restart
// Wait a moment for the user to see the message, then restart the app - await Future.delayed(const Duration(seconds: 2)); - - // Restart the app to apply cron expression changes - if (mounted) { - Restart.restartApp(); - } + if (mounted) { + final shouldRestart = await showDialog<bool>( + context: context, + builder: (ctx) => AlertDialog( + title: const Text('Restart Required'), + content: const Text('The app needs to restart to apply cron expression changes. Restart now?'), + actions: [ + TextButton( + onPressed: () => Navigator.of(ctx).pop(false), + child: const Text('Later'), + ), + TextButton( + onPressed: () => Navigator.of(ctx).pop(true), + child: const Text('Restart'), + ), + ], + ), + ); + if (shouldRestart == true) { + Restart.restartApp(); + } + }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
android/app/src/main/java/io/mosip/registration_client/MainActivity.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/app/src/main/java/io/mosip/registration_client/utils/BatchJob.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/JobManagerServiceImpl.javalib/platform_android/sync_response_service_impl.dartlib/platform_spi/sync_response_service.dartlib/ui/settings/widgets/scheduled_jobs_settings.dartpigeon/master_data_sync.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/platform_android/sync_response_service_impl.dart
🧰 Additional context used
🧬 Code graph analysis (2)
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/util/CronExpressionParser.java (1)
CronExpressionParser(19-65)android/app/src/main/java/io/mosip/registration_client/UploadBackgroundService.java (1)
UploadBackgroundService(20-86)
android/app/src/main/java/io/mosip/registration_client/MainActivity.java (1)
android/app/src/main/java/io/mosip/registration_client/UploadBackgroundService.java (1)
UploadBackgroundService(20-86)
🔇 Additional comments (11)
android/app/src/main/java/io/mosip/registration_client/MainActivity.java (1)
234-237: LGTM!The receiver registration is correct and the comment clearly explains its purpose.
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (4)
439-448: LGTM!The
getPermittedJobsimplementation correctly handles null by returning an empty list, and exceptions are properly logged and propagated.
450-459: LGTM!The
isValidCronExpressionimplementation safely delegates toCronExpressionParser.isValidCronExpressionand returnsfalseon any exception, which is appropriate for a validation method.
498-507: LGTM!The
getValueimplementation properly delegates toLocalConfigServicewith appropriate error handling and logging.
481-489: No action needed—the "RESCHEDULE_JOB" broadcast receiver is properly registered in MainActivity.java (lines 236-237) with a corresponding BroadcastReceiver implementation (lines 217-226) that extracts the job API name and calls createBackgroundTask() to reschedule. The broadcast will be received and handled correctly.Likely an incorrect or invalid review comment.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/JobManagerServiceImpl.java (1)
193-206: LGTM!The
getSyncFrequencyhelper method correctly prioritizes local configuration overrides while safely falling back to the default sync frequency. The null and empty string checks are appropriate.android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java (1)
213-226: LGTM!The custom cron expression lookup follows the same pattern as
JobManagerServiceImpl.getSyncFrequency(), ensuring consistent behavior across scheduling components. The debug logging is helpful for troubleshooting which cron source is being used.pigeon/master_data_sync.dart (1)
66-73: LGTM!The new Pigeon API methods are well-defined with appropriate return types and align with the corresponding Android implementations in
MasterDataSyncApi.lib/platform_spi/sync_response_service.dart (1)
35-38: LGTM!The abstract method signatures correctly mirror the Pigeon API definitions and provide the appropriate interface for the platform implementation.
lib/ui/settings/widgets/scheduled_jobs_settings.dart (2)
29-53: LGTM on permitted jobs loading.The state management for loading permitted jobs is well-structured with proper error handling and loading state tracking.
347-400: LGTM on conditional cron editing UI.The UI correctly renders an editable text field with validation for permitted jobs and falls back to a static display for non-permitted jobs. The error styling and submit button are appropriately implemented.
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/provider/sync_provider.dart (2)
55-56: Returning mutable internal map exposes state to external modification.The getter returns the internal
_jobStatusesmap directly, allowing callers to mutate provider state without triggeringnotifyListeners().🔎 Proposed fix
- Map<String, JobStatus> get jobStatuses => _jobStatuses; + Map<String, JobStatus> get jobStatuses => Map.unmodifiable(_jobStatuses);
303-326: Sequential async calls in loop may slow polling with many jobs.Each job iteration awaits
getLastSyncTimeByJobIdandgetNextSyncTimeByJobIdsequentially. With N jobs, this results in 2N serial async operations.Consider parallelizing per-job fetches if job count grows or latency becomes noticeable.
🔎 Proposed parallel approach
Future<void> refreshJobStatuses() async { try { final activeJobs = await syncResponseService.getActiveSyncJobs(); + final futures = <Future<void>>[]; for (final jobJson in activeJobs) { - if (jobJson == null) continue; - try { - final job = SyncJobDef.fromJson(json.decode(jobJson) as Map<String, dynamic>); - if (job.id != null) { - final lastSync = await getLastSyncTimeByJobId(job.id!); - final nextSync = await getNextSyncTimeByJobId(job.id!); - - _jobStatuses[job.id!] = JobStatus(id: job.id!, lastSyncTime: lastSync, nextSyncTime: nextSync); - } - } catch (e) { - log("Error parsing job during polling: $e"); - } + if (jobJson == null) continue; + futures.add(_fetchAndUpdateJobStatus(jobJson)); } + await Future.wait(futures); await getLastSyncTime(); // Update global last sync time (fallback for Master Sync) notifyListeners(); } catch (e) { log("Error refreshing job statuses: $e"); } } + + Future<void> _fetchAndUpdateJobStatus(String jobJson) async { + try { + final job = SyncJobDef.fromJson(json.decode(jobJson) as Map<String, dynamic>); + if (job.id != null) { + final results = await Future.wait([ + getLastSyncTimeByJobId(job.id!), + getNextSyncTimeByJobId(job.id!), + ]); + _jobStatuses[job.id!] = JobStatus( + id: job.id!, + lastSyncTime: results[0], + nextSyncTime: results[1], + ); + } + } catch (e) { + log("Error parsing job during polling: $e"); + } + }lib/ui/settings/widgets/scheduled_jobs_settings.dart (1)
59-74: Store provider reference to avoid context access in deactivate.The comments in
dispose()reveal uncertainty about lifecycle handling. Accessingcontext.readindeactivate()works but is fragile. Store the provider reference ininitStatefor safer cleanup.🔎 Proposed fix
class _ScheduledJobsSettingsState extends State<ScheduledJobsSettings> { List<String?> _permittedJobs = []; bool _isLoadingPermittedJobs = true; + late SyncProvider _syncProvider; @override void initState() { super.initState(); + _syncProvider = context.read<SyncProvider>(); WidgetsBinding.instance.addPostFrameCallback((_) { - context.read<SyncProvider>().startJobPolling(); + _syncProvider.startJobPolling(); }); _loadPermittedJobs(); } @override void dispose() { - // Only stop polling if we are sure we started it and valid context access - // But since context might be invalid, we skip explicitly stopping here - // unless we store the provider reference. - // Actually, let's store it in initState or just accept it runs? - // Best practice: Access provider in didChangeDependencies or similar? - // For now, let's leave start in initState and we can add a deactivate hook. + _syncProvider.stopJobPolling(); super.dispose(); } - - @override - void deactivate() { - context.read<SyncProvider>().stopJobPolling(); - super.deactivate(); - }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/provider/sync_provider.dartlib/ui/settings/widgets/scheduled_jobs_settings.dart
🔇 Additional comments (6)
lib/provider/sync_provider.dart (2)
290-301: LGTM!Timer lifecycle is correctly managed:
startJobPollingcancels any existing timer before creating a new one, anddisposeproperly stops polling.Also applies to: 328-332
335-341: LGTM!Simple immutable data class with appropriate structure for job status tracking.
lib/ui/settings/widgets/scheduled_jobs_settings.dart (4)
240-248: App restart is disruptive UX—verify if necessary.Restarting the entire app after a cron change can cause loss of unsaved user state and is jarring. Confirm whether the platform layer truly requires an app restart to apply cron expression changes, or if jobs can be rescheduled dynamically.
339-355: LGTM!Good use of
Consumer<SyncProvider>to reactively display job statuses with appropriate fallback handling for the masterSyncJob case.
356-409: LGTM!Cron editing UI is properly gated behind
isPermittedcheck with inline validation feedback.
158-170: LGTM!Correct precedence for loading cron expression: custom value first, then default from job definition.
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @lib/ui/settings/widgets/scheduled_jobs_settings.dart:
- Around line 158-170: The _loadCronExpression method can throw from
_syncResponseService.getValue and leave _cronController.text stale; wrap the
getValue call in a try-catch, set a fallback (_cronController.text =
widget.job.syncFreq ?? '') in the catch and also ensure the controller is
assigned in the non-exception path (after the await), and log or handle the
caught exception; update references in _loadCronExpression,
_syncResponseService.getValue, and _cronController accordingly.
- Around line 60-74: The fix: avoid calling context.read<SyncProvider>() in
deactivate; instead obtain and cache the provider reference in
didChangeDependencies (e.g. assign to a private field like _syncProvider of type
SyncProvider or ProviderListener) and then call _syncProvider.stopJobPolling()
from dispose where context is safe to access during teardown; remove or replace
the context.read call in deactivate (no direct context access there) and ensure
the cached field is nullable/checked if necessary before calling stopJobPolling
to avoid exceptions.
- Around line 357-359: The code sets lastSync via
formatDate(provider.lastSuccessfulSyncTime) when widget.job.apiName ==
"masterSyncJob" but doesn't check for an empty lastSuccessfulSyncTime, which
yields a blank display; update the branch in the scheduled jobs widget to first
check provider.lastSuccessfulSyncTime (or .isNotEmpty) and only call formatDate
if present, otherwise assign a fallback string like "Never synced" to lastSync
so the UI shows a meaningful message; change the logic around
widget.job.apiName, lastSync, provider.lastSuccessfulSyncTime and formatDate
accordingly.
- Around line 256-261: The Restart.restartApp() call will fail on iOS because
the restart_app package requires a CFBundleURLTypes entry in Info.plist; update
the iOS app’s Info.plist to add a CFBundleURLTypes array with a dictionary
including a CFBundleURLName and CFBundleURLSchemes (a unique scheme for the app)
so the package can trigger the restart, then rebuild the iOS app and verify
Restart.restartApp() successfully restarts the app on iOS.
🧹 Nitpick comments (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
126-131: Consider removing unused parameter.The
configTypeparameter is passed but never used within the method body. This reduces code clarity.🔎 Proposed refactor
-private void updateLocalPreference(LocalPreferences localPreference, String configType) { +private void updateLocalPreference(LocalPreferences localPreference) { localPreference.setIsDeleted(true); localPreference.setUpdBy(RegistrationConstants.JOB_TRIGGER_POINT_USER); localPreference.setUpdDtimes(System.currentTimeMillis()); localPreferencesRepository.save(localPreference); }And update the call site at line 110:
-updateLocalPreference(localPreferences, RegistrationConstants.PERMITTED_JOB_TYPE); +updateLocalPreference(localPreferences);lib/ui/settings/widgets/scheduled_jobs_settings.dart (1)
381-396: Consider usingerrorTextinstead of customerrorBorderlogic.Setting
errorText: nullwhile conditionally applyingerrorBorderbased on_cronErroris inconsistent. The built-inerrorTextparameter would simplify the decoration and provide consistent Material Design styling.🔎 Proposed refactor
child: TextField( controller: _cronController, decoration: InputDecoration( hintText: 'Cron Expression', - errorText: null, - errorBorder: _cronError != null - ? const OutlineInputBorder( - borderSide: BorderSide(color: Colors.red, width: 1)) - : null, border: const OutlineInputBorder(), isDense: true, contentPadding: const EdgeInsets.symmetric(horizontal: 8, vertical: 8), ), style: const TextStyle(fontSize: 11), ), ), - if (_cronError != null) - Padding( - padding: const EdgeInsets.only(top: 1.0, left: 4.0), - child: Text( - _cronError!, - style: const TextStyle(fontSize: 9, color: Colors.red, height: 1), - ), - ),If you prefer keeping the custom error display, at least remove the
errorText: nullline as it's redundant.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/RoomModule.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.javalib/ui/settings/widgets/scheduled_jobs_settings.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/util/CronExpressionParser.java (1)
CronExpressionParser(19-65)
🔇 Additional comments (8)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/RoomModule.java (1)
472-478: LGTM! Proper dependency injection for transactional support.The addition of
ClientDatabaseparameter enables the DAO layer to perform atomic transaction operations (viarunInTransaction()), which is essential for ensuring data consistency in job modification operations.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java (3)
43-45: LGTM! Clean delegation to DAO layer.The
getValuemethod appropriately delegates to the DAO layer for data retrieval.
48-65: Excellent validation before persistence!The
modifyJobmethod implements proper defensive validation:
- Rejects null/empty values early
- Validates cron expression syntax using
CronExpressionParser- Provides clear error messages for debugging
- Only delegates to DAO after validation passes
This prevents invalid cron expressions from being persisted, which would cause job scheduling failures at runtime.
68-70: LGTM! Clean delegation for permitted jobs retrieval.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (3)
26-26: LGTM! ClientDatabase injection enables transactional operations.The
ClientDatabasefield is properly injected and stored, enabling atomic transaction support for job modifications viarunInTransaction().Also applies to: 31-31, 34-34
85-95: LGTM! Clean value retrieval with error handling.The
getValuemethod safely retrieves configuration values with null checks and error logging.
98-121: Excellent atomic transaction handling!The
modifyJobmethod properly wraps the soft-delete and create operations in a single transaction usingclientDatabase.runInTransaction(). This ensures:
- Atomicity: Both operations succeed or both roll back
- Thread safety: Transaction boundaries prevent race conditions
- Data consistency: No partial updates can occur
The exception handling correctly re-throws to trigger rollback on failure.
lib/ui/settings/widgets/scheduled_jobs_settings.dart (1)
220-232: Good defensive validation for job ID.The null/empty check for job ID before calling
modifyJobCronExpressionproperly addresses the previous review feedback and prevents undefined behavior.
...anager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java:
- Around line 121-129: The method updateLocalPreference(LocalPreferences
localPreference, String configType) contains an unused parameter configType;
remove the unused parameter from the method signature in LocalConfigDAOImpl
(change to updateLocalPreference(LocalPreferences localPreference)), update any
internal references if present, and then update all call sites (e.g., the caller
around the previous call at line ~108) to invoke updateLocalPreference with only
the LocalPreferences argument; keep the existing body (setting isDeleted, updBy,
updDtimes, and saving via localPreferencesRepository) unchanged.
In @lib/ui/settings/widgets/scheduled_jobs_settings.dart:
- Around line 318-319: The async callback in the ScheduledJobsSettings widget
calls context.read<SyncProvider>().refreshJobStatuses() after an await without
verifying widget lifecycle; add a mounted check before accessing context by
returning early if (!mounted) or wrapping the context.read call in if (mounted)
{ context.read<SyncProvider>().refreshJobStatuses(); } so the call only runs
when the State is still mounted.
🧹 Nitpick comments (7)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
84-95: Consider distinguishing between "not found" and error cases.The method returns
nullfor both scenarios where the preference doesn't exist and when an exception occurs. Callers cannot differentiate between these cases, which might complicate error handling upstream.If the distinction matters, consider either throwing the exception or returning an
Optional<String>with more specific error signaling.lib/ui/settings/widgets/scheduled_jobs_settings.dart (6)
175-187: Consider returning a more user-friendly fallback.The
formatDatemethod returns the originaldateStringon error, which might display a malformed or raw timestamp in the UI. Consider returning a placeholder like"Invalid date"or"-"for better UX.🔎 Optional improvement
} catch(e) { - return dateString; + return "-"; }
252-258: Add loading indicator during async cron modification.The 2-second delay before restart happens without a loading indicator, which could confuse users. Additionally, if the user navigates away during this period, the app restart might be unexpected.
Consider:
- Showing a loading dialog during the save/restart sequence
- Disabling the Submit button and showing a progress indicator
- Preventing navigation during the critical period
🔎 Proposed improvement with loading dialog
if (success && mounted) { // Clear error setState(() { _cronError = null; }); - // Show success message - ScaffoldMessenger.of(context).showSnackBar( - const SnackBar( - content: Text('Cron expression saved successfully. Restarting app...'), - duration: Duration(seconds: 2), - ), - ); - - // Wait a moment for the user to see the message, then restart the app - await Future.delayed(const Duration(seconds: 2)); + // Show loading dialog + if (mounted) { + showDialog( + context: context, + barrierDismissible: false, + builder: (context) => const AlertDialog( + content: Row( + children: [ + CircularProgressIndicator(), + SizedBox(width: 16), + Text('Restarting app...'), + ], + ), + ), + ); + } + + // Brief delay to show the dialog + await Future.delayed(const Duration(milliseconds: 500)); // Restart the app to apply cron expression changes if (mounted) { Restart.restartApp(); }
409-416: Disable Submit button during async operation.The Submit button remains enabled while
_modifyCronExpressionis executing, allowing users to tap it multiple times and trigger concurrent save operations. This could lead to race conditions or duplicate API calls.🔎 Proposed fix with loading state
Add a loading state field:
class _JobCardState extends State<_JobCard> { String? _lastSync; String? _nextSync; late SyncProvider syncProvider; final TextEditingController _cronController = TextEditingController(); final SyncResponseService _syncResponseService = SyncResponseService(); String? _cronError; + bool _isSaving = false;Then wrap the button logic:
SizedBox( height: 32, width: 65, child: ElevatedButton( - onPressed: _modifyCronExpression, + onPressed: _isSaving ? null : _modifyCronExpression, style: ElevatedButton.styleFrom( padding: EdgeInsets.zero, ), - child: const Text('Submit', style: TextStyle(fontSize: 11)), + child: _isSaving + ? const SizedBox( + width: 16, + height: 16, + child: CircularProgressIndicator(strokeWidth: 2), + ) + : const Text('Submit', style: TextStyle(fontSize: 11)), ), ),And update
_modifyCronExpression:Future<void> _modifyCronExpression() async { + setState(() { + _isSaving = true; + }); + final cronExpression = _cronController.text.trim(); if (cronExpression.isEmpty) { setState(() { _cronError = 'Cron expression cannot be empty'; + _isSaving = false; }); return; }(Apply similar pattern to all early returns and the finally block)
30-30: Consider using Set for _permittedJobs to improve lookup performance.The code uses
_permittedJobs.contains(job.id)for permission checks, which has O(n) complexity on a List. If the number of permitted jobs grows, consider using a Set for O(1) lookups.🔎 Proposed optimization
class _ScheduledJobsSettingsState extends State<ScheduledJobsSettings> { - List<String?> _permittedJobs = []; + Set<String?> _permittedJobs = {}; bool _isLoadingPermittedJobs = true; SyncProvider? _syncProvider;Then in
_loadPermittedJobs:setState(() { - _permittedJobs = permittedJobs; + _permittedJobs = permittedJobs.toSet(); _isLoadingPermittedJobs = false; });Also applies to: 109-109
116-116: Extract magic number for bottom padding.The bottom padding calculation includes a magic number
230which makes the intent unclear. Consider extracting this as a named constant for better maintainability.🔎 Proposed improvement
+// Extra padding to account for expanded job cards +const double _kJobCardBottomPadding = 230.0; + class ScheduledJobsSettings extends StatefulWidget { ... } ... - SliverToBoxAdapter(child: SizedBox(height: MediaQuery.of(context).padding.bottom + kBottomNavigationBarHeight + 230)), + SliverToBoxAdapter(child: SizedBox(height: MediaQuery.of(context).padding.bottom + kBottomNavigationBarHeight + _kJobCardBottomPadding)),
189-272: Consider extracting validation and save logic to separate methods.The
_modifyCronExpressionmethod handles validation, saving, user feedback, and app restart in a single function. Consider extracting validation and save operations into separate methods for better testability and maintainability.🔎 Example refactoring
Future<bool> _validateCronExpression(String cronExpression) async { if (cronExpression.isEmpty) { setState(() { _cronError = 'Cron expression cannot be empty'; }); return false; } final isValid = await _syncResponseService.isValidCronExpression(cronExpression); if (!isValid) { setState(() { _cronError = 'Invalid cron expression'; }); if (mounted) { ScaffoldMessenger.of(context).showSnackBar( const SnackBar(content: Text('Invalid cron expression')), ); } return false; } setState(() { _cronError = null; }); return true; } Future<bool> _saveCronExpression(String jobId, String cronExpression) async { try { return await _syncResponseService.modifyJobCronExpression( jobId, cronExpression, ); } catch (e) { debugPrint('Error modifying cron expression: $e'); if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar(content: Text('Error: $e')), ); } return false; } } Future<void> _modifyCronExpression() async { final cronExpression = _cronController.text.trim(); // Validate expression if (!await _validateCronExpression(cronExpression)) { return; } // Validate job ID final jobId = widget.job.id; if (jobId == null || jobId.isEmpty) { setState(() { _cronError = 'Job ID is required'; }); if (mounted) { ScaffoldMessenger.of(context).showSnackBar( const SnackBar(content: Text('Cannot save cron expression: Job ID is missing')), ); } return; } // Save and restart final success = await _saveCronExpression(jobId, cronExpression); if (success && mounted) { // Show success and restart // ... (existing restart logic) } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javalib/ui/settings/widgets/scheduled_jobs_settings.dart
🧰 Additional context used
🧬 Code graph analysis (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
RegistrationConstants(9-139)
🔇 Additional comments (6)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (2)
13-13: LGTM: Dependency injection properly implemented.The
ClientDatabasedependency is correctly injected via constructor and stored as a private field, following Dagger DI best practices with@Injectand@Singletonannotations.Also applies to: 26-26, 30-34
97-119: LGTM: Transaction handling correctly implemented.The method now wraps both the soft-delete and create operations in
clientDatabase.runInTransaction(), ensuring atomicity and addressing the previous review concern. The re-throw pattern correctly triggers transaction rollback on failure.lib/ui/settings/widgets/scheduled_jobs_settings.dart (4)
149-167: LGTM - Error handling properly implemented.The
_loadCronExpressionmethod now correctly handles errors fromgetValuewith a try-catch block and falls back to the default cron expression. This addresses the previous review concern.
350-366: Dynamic job status display looks good.The
Consumer<SyncProvider>properly rebuilds when job statuses change, and the special handling formasterSyncJobcorrectly uses the globallastSuccessfulSyncTime. TheformatDatemethod handles empty or invalid values gracefully.
443-453: LGTM - Clean key-value display helper.The
_kvhelper method is well-structured with proper overflow handling viaFlexible. The styling is consistent and appropriate for the UI.
37-40: ThestartJobPolling()implementation already includes duplicate-start protection by cancelling any existing timer before creating a new one (line 292 inlib/provider/sync_provider.dart). No changes needed.
.../clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/ui/settings/widgets/scheduled_jobs_settings.dart (1)
377-394: Consider clearing validation errors as user types.The TextField displays validation errors via
_cronError, but errors persist until the user submits again. Adding anonChangedcallback to clear_cronErrorwhen the user starts editing would provide better immediate feedback.🔎 Proposed enhancement
TextField( controller: _cronController, + onChanged: (value) { + if (_cronError != null) { + setState(() { + _cronError = null; + }); + } + }, decoration: InputDecoration( hintText: 'Cron Expression',
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javalib/ui/settings/widgets/scheduled_jobs_settings.dart
🧰 Additional context used
🧬 Code graph analysis (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
RegistrationConstants(9-139)
🔇 Additional comments (7)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (2)
26-35: LGTM!Clean dependency injection of
ClientDatabaseto enable transactional operations. The constructor properly initializes all fields.
84-95: LGTM!The method has proper null safety with checks on both the entity and its value. Error handling with logging and null fallback is appropriate for a retrieval method.
lib/ui/settings/widgets/scheduled_jobs_settings.dart (5)
29-65: Well-structured state management and lifecycle handling.The implementation correctly:
- Uses
addPostFrameCallbackto safely access context during initialization- Stores provider reference to avoid context access issues in
dispose- Includes mounted checks before
setStatein async operations- Handles errors gracefully in
_loadPermittedJobsThis addresses previous concerns about context access in
deactivateand ensures clean polling lifecycle management.
149-167: Good error handling for cron expression loading.The
_loadCronExpressionmethod properly:
- Wraps async operations in try-catch
- Provides fallback to default cron expression on error
- Validates job ID before making service calls
- Logs errors for debugging
This addresses the previous concern about unhandled exceptions from
getValue.
217-229: Proper validation of job ID before save operation.The validation correctly checks for null or empty job ID before calling
modifyJobCronExpression, providing clear user feedback via SnackBar and preventing invalid operations. This addresses the previous concern about passing empty IDs to the service.
318-320: Proper mounted check before context access.The mounted check before calling
context.read<SyncProvider>()prevents exceptions if the widget is disposed during the async sync operation. Good defensive programming.
425-437: Manual sync button does not needisPermittedcheck — this is correct by design.The
isPermittedflag controls permission to edit cron schedules (lines 368-407), not to trigger manual sync operations. These are separate permissions: manual sync (triggered by the button at lines 425-437) is available for all non-packet jobs, while cron editing is restricted to permitted jobs. This design is consistent with the permission model used elsewhere in the codebase (e.g., global config editing).Likely an incorrect or invalid review comment.
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/ui/settings/widgets/scheduled_jobs_settings.dart (2)
44-58: Consider showing user feedback when permitted jobs fail to load.If
getPermittedJobs()throws an exception, the error is logged but the UI shows an empty permitted jobs list without any indication to the user that loading failed. Consider displaying a SnackBar or error message so users understand why no jobs appear as editable.🔎 Proposed fix
Future<void> _loadPermittedJobs() async { try { final service = SyncResponseService(); final permittedJobs = await service.getPermittedJobs(); if (!mounted) return; setState(() { _permittedJobs = permittedJobs; _isLoadingPermittedJobs = false; }); } catch (e) { debugPrint('Failed to load permitted jobs: $e'); + if (mounted) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Failed to load job permissions: $e')), + ); + } setState(() { _isLoadingPermittedJobs = false; }); } }
150-168: Consider showing user feedback when cron expression fails to load.While the try-catch properly handles errors and falls back to the default cron expression, users are not informed when loading fails. This could lead to confusion if the custom cron expression exists but fails to load, causing the UI to display the default value instead.
🔎 Proposed fix
Future<void> _loadCronExpression() async { try { if (widget.job.id != null && widget.job.id!.isNotEmpty) { final customCron = await _syncResponseService.getValue(widget.job.id!); if (customCron != null && customCron.trim().isNotEmpty) { _cronController.text = customCron; } else { _cronController.text = widget.job.syncFreq ?? ''; } } else { _cronController.text = widget.job.syncFreq ?? ''; } } catch (e) { debugPrint('Failed to load cron expression: $e'); + if (mounted) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Failed to load custom cron expression: $e')), + ); + } _cronController.text = widget.job.syncFreq ?? ''; } }
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java
Show resolved
Hide resolved
android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java
Outdated
Show resolved
Hide resolved
.../clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
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/dao/SyncJobDefDao.java (1)
51-51: Fix operator precedence in the existing query.Line 51 has the same SQL operator precedence issue that was fixed in Line 34. The current query
where is_deleted == 0 or is_deleted is null and is_active=:isActivewill be parsed as(is_deleted == 0) or (is_deleted is null and is_active=:isActive), which may return deleted records.🔒 Proposed fix
- @Query("select * from sync_job_def where is_deleted == 0 or is_deleted is null and is_active=:isActive") + @Query("select * from sync_job_def where (is_deleted == 0 or is_deleted is null) and is_active=:isActive")
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
android/app/src/main/java/io/mosip/registration_client/HostApiModule.javaandroid/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/app/src/main/java/io/mosip/registration_client/utils/BatchJob.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/SyncJobDefDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/SyncJobDefRepository.java
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/io/mosip/registration_client/HostApiModule.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/util/CronExpressionParser.java (1)
CronExpressionParser(19-65)android/app/src/main/java/io/mosip/registration_client/UploadBackgroundService.java (1)
UploadBackgroundService(20-86)
🔇 Additional comments (14)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/SyncJobDefDao.java (2)
31-35: LGTM! Critical operator precedence fix.The added parentheses correctly enforce the intended query logic:
(is_deleted == 0 or is_deleted is null) and id=:jobId. Without parentheses, SQL would evaluate asis_deleted == 0 or (is_deleted is null and id=:jobId), potentially returning wrong results.
37-44: LGTM! Consistent implementation pattern.The new
findOneByApiNamemethod correctly applies the same parenthesized condition pattern as the fixedfindOneByIdquery.android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (6)
62-62: LGTM! Proper dependency injection.LocalConfigService is correctly integrated through constructor injection and field assignment following the existing DI pattern.
Also applies to: 105-105, 126-126, 152-152
439-448: LGTM! Safe null handling.The method returns an empty list when
permittedJobsis null, preventing potential NPEs in callers.
450-459: LGTM! Defensive exception handling.The method gracefully catches validation errors and returns
false, ensuring the API never throws to Pigeon callers.
491-500: LGTM! Straightforward delegation.The method delegates to
LocalConfigServicewith appropriate error handling.
586-596: LGTM! Improved error handling.The refactored implementation now uses
getSyncJobDefByApiNameand properly checks for null before accessing the ID, avoiding potential NPEs.
461-489: The code is correct.MainActivityhas a properly registeredBroadcastReceiverfor the"RESCHEDULE_JOB"action (lines 236-237 in MainActivity), and it is properly implemented (lines 217-226) to handle job rescheduling by callingcreateBackgroundTask()with the job API name extracted from the broadcast intent.Likely an incorrect or invalid review comment.
android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java (2)
26-26: LGTM! Proper dependency injection.LocalConfigService is correctly integrated following the existing constructor injection pattern.
Also applies to: 37-37, 42-49
207-228: LGTM! Clean custom cron override logic.The implementation correctly:
- Fetches the default cron from the database
- Checks if
localConfigServiceis available- Overrides with custom cron only if non-null and non-empty
- Logs both default and custom cron expressions for debugging
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/SyncJobDefRepository.java (1)
40-58: LGTM! Clean repository accessors.Both methods are straightforward DAO delegations with clear Javadoc documenting the null-return behavior when records are not found.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (3)
13-13: LGTM! Transactional support added.
ClientDatabaseis correctly injected to enable transactional operations in the newmodifyJobmethod.Also applies to: 26-26, 29-35
84-95: LGTM! Safe value retrieval.The method handles exceptions gracefully and returns null when the preference is not found or an error occurs.
97-122: LGTM! Proper transactional semantics.The method correctly wraps the read-check-write sequence in
clientDatabase.runInTransactionto ensure atomicity and prevent race conditions. Re-throwing asRuntimeException(Line 119) properly triggers transaction rollback on failure.
.../clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java:
- Around line 93-112: The modifyJob method swallows all exceptions and lacks
input validation; change it to validate inputs (ensure name and value are
non-null/non-empty) at the start, avoid catching broad Exception (or rethrow
after logging) and instead propagate failure to callers by throwing a specific
runtime exception or returning a boolean status; update callers accordingly (or
document the thrown exception), and ensure saveLocalPreference and
localPreferencesRepository.save calls are executed only after validation so
failures are not silently logged in modifyJob.
- Around line 93-112: modifyJob duplicates the update/create logic used in
modifyConfigurations; extract the shared logic into a private helper (e.g.,
upsertLocalPreference or updateOrCreateLocalPreference) that accepts name, value
and configType, reuse it from both modifyJob and modifyConfigurations, and call
saveLocalPreference from within that helper with the appropriate
RegistrationConstants value; update modifyJob to call the helper instead of
repeating the block and keep exception logging as-is.
- Around line 93-112: LocalConfigServiceImpl.modifyJob currently validates cron
expressions but does not check whether the provided name is a permitted job;
before calling the DAO (or delegating to LocalConfigDAOImpl.modifyJob), call
getPermittedJobs() and if it does not contain name, log an error and throw
IllegalArgumentException (use the suggested message), so only jobs listed by
getPermittedJobs() can be modified.
🧹 Nitpick comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
80-91: Consider distinguishing between "not found" and "error" cases.The method returns
nullfor both scenarios: when the preference doesn't exist and when an exception occurs. This makes it difficult for callers to distinguish between legitimate absence and an error condition. Additionally, there's no validation thatnameis non-null or non-empty before querying.Consider either:
- Using Optional to better express absence
- Throwing an unchecked exception for errors while returning null only for "not found"
- Adding input validation for the
nameparameter♻️ Possible approaches
Approach 1: Add input validation
@Override public String getValue(String name) { + if (name == null || name.trim().isEmpty()) { + Log.w(TAG, "getValue called with invalid name"); + return null; + } try { LocalPreferences localPreference = localPreferencesRepository.findByIsDeletedFalseAndName(name); if (localPreference != null && localPreference.getVal() != null) { return localPreference.getVal(); } } catch (Exception e) { Log.e(TAG, "Error getting value for: " + name, e); + throw new RuntimeException("Failed to retrieve configuration: " + name, e); } return null; }Approach 2: Use Optional (if project supports Java 8+)
+import java.util.Optional; + @Override -public String getValue(String name) { +public Optional<String> getValue(String name) { try { LocalPreferences localPreference = localPreferencesRepository.findByIsDeletedFalseAndName(name); - if (localPreference != null && localPreference.getVal() != null) { - return localPreference.getVal(); - } + return Optional.ofNullable(localPreference) + .map(LocalPreferences::getVal); } catch (Exception e) { Log.e(TAG, "Error getting value for: " + name, e); + throw new RuntimeException("Failed to retrieve configuration: " + name, e); } - return null; + return Optional.empty(); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
RegistrationConstants(9-144)
.../clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
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/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
135-163:cleanUpLocalPreferences()can NPE on null permitted configs and null active flags.
permittedConfigsis not null-checked before theforloop on Line 142.config.getIsActive()can be null; then!permittedStatusMap.get(key)on Line 154 can throw due to Boolean unboxing.Proposed fix (null-safe)
public void cleanUpLocalPreferences() { List<PermittedLocalConfig> permittedConfigs = permittedLocalConfigRepository .getPermittedConfigsByType(RegistrationConstants.PERMITTED_CONFIG_TYPE); Map<String, String> localConfigs = getLocalConfigurations(); Map<String, Boolean> permittedStatusMap = new java.util.HashMap<>(); - for (PermittedLocalConfig config : permittedConfigs) { - permittedStatusMap.put(config.getName(), config.getIsActive()); - } + if (permittedConfigs != null) { + for (PermittedLocalConfig config : permittedConfigs) { + permittedStatusMap.put(config.getName(), Boolean.TRUE.equals(config.getIsActive())); + } + } - for (String key : localConfigs.keySet()) { + if (localConfigs == null || localConfigs.isEmpty()) return; + for (String key : localConfigs.keySet()) { LocalPreferences pref = localPreferencesRepository.findByIsDeletedFalseAndName(key); if (pref == null) continue; if (!permittedStatusMap.containsKey(key)) { localPreferencesRepository.delete(pref); Log.i(TAG, "Local preference deleted (row removed): " + key); - } else if (!permittedStatusMap.get(key)) { + } else if (Boolean.FALSE.equals(permittedStatusMap.get(key))) { // Key deactivated, mark as deleted pref.setIsDeleted(true); pref.setUpdBy(RegistrationConstants.JOB_TRIGGER_POINT_USER); pref.setUpdDtimes(System.currentTimeMillis()); localPreferencesRepository.save(pref); Log.i(TAG, "Local preference marked deleted (row deactivated): " + key); } } }
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java:
- Around line 80-95: The DAO is vulnerable to cross-contamination because
lookups ignore configType; add a repository/DAO method
findByIsDeletedFalseAndNameAndConfigType(String name, String configType) and use
it in LocalConfigDAOImpl: update saveOrUpdateLocalPreference(...) to call
findByIsDeletedFalseAndNameAndConfigType(name, configType) instead of
findByIsDeletedFalseAndName(name); similarly change getValue(...) to use
findByIsDeletedFalseAndNameAndConfigType(name,
RegistrationConstants.PERMITTED_JOB_TYPE) (or appropriate configType) and update
cleanUpLocalPreferences(...) to filter by configType via the new
findByIsDeletedFalseAndNameAndConfigType method so JOB and CONFIGURATION records
are looked up/modified separately.
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java:
- Around line 47-70: modifyJob currently only validates the cron value; first
add explicit validation for the job name (null or blank) at the start of
modifyJob(String name, String value) — if name is null or empty, Log.e with a
clear message and throw an IllegalArgumentException like "Job name cannot be
null or empty" before any permitted-job checks so callers get the correct
failure reason; keep the existing cron validation and permitted-job check order
after the name validation and only call localConfigDAO.modifyJob(name, value) if
all validations pass.
🧹 Nitpick comments (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (2)
53-65: Avoid persisting non-permitted keys inmodifyConfigurations()(or document the contract).On Line 55–64 you now upsert every entry via
saveOrUpdateLocalPreference(...)without checking thatnameis in the permitted CONFIGURATION list. If upstream isn’t already enforcing this, you can end up storing keys that will later be deleted/marked-deleted by cleanup (and create surprising UX).Proposed adjustment (validate permitted keys once)
public void modifyConfigurations(Map<String, String> localPreferences) { + List<String> permitted = getPermittedConfigurations(RegistrationConstants.PERMITTED_CONFIG_TYPE); for (Map.Entry<String, String> entry : localPreferences.entrySet()) { String name = entry.getKey(); String value = entry.getValue(); try { + if (permitted.contains(name)) { saveOrUpdateLocalPreference(name, value, RegistrationConstants.PERMITTED_CONFIG_TYPE); + } else { + Log.w(TAG, "Ignoring non-permitted configuration key: " + name); + } } catch (Exception e) { Log.e(TAG, "Error modifying configuration: " + name, e); } } }Also applies to: 97-113
67-78:getValue()swallowing exceptions may mask real storage failures.On Line 74–77 you log and return null for any exception. Callers can’t distinguish “missing key” vs “DB error”. If null is the intended API, consider returning an
Optional-like wrapper (if feasible) or adding a dedicated error signal at the service boundary.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java (1)
67-70: Comment about “transaction-safe persistence” looks inaccurate (or needs backing).On Line 68–69: the DAO shown uses simple
find...+save(...)without any visible transaction boundary. Either tighten the comment or add a transaction mechanism if the underlying stack supports it.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java
🔇 Additional comments (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java (2)
42-46: Nice layering: service delegates reads and permitted-job retrieval cleanly.
getValue()andgetPermittedJobs()are thin delegations, which keeps validation centralized in the service while persistence stays in the DAO.Also applies to: 72-75
14-15: Exception handling is already correct; cron expression is not used for actual scheduling.
isValidCronExpression()properly catches all exceptions and returnsfalserather than throwing—callers won't see thrown exceptions. More significantly, thesyncFreqparameter passed toscheduleJob()(line 100) is accepted but never used. Actual scheduling uses a hardcoded 15-minute interval (JOB_PERIODIC_SECONDS, line 109–113), not the cron expression. Cron validation is only applied for UI display of next sync time calculation (line 177–181) and pre-validation inLocalConfigServiceImpl.modifyJob()(line 57–59). There is no dialect mismatch with the scheduler because the validated cron expression never reaches the scheduler; Android'sJobSchedulerdoes not support cron expressions.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
97-113: This concern is incorrect—the database schema does not enforce uniqueness constraints onnameor(name, configType).The
local_preferencestable (from ClientDatabaseMigrations.java) only has a PRIMARY KEY onid. Eachinsert()call uses a new UUID for the id field, so there is no primary key conflict. Multiple rows with the samenamecan safely coexist with different id values.The code uses soft deletes (setting
is_deleted = 1), which means deleted records remain in the database. While resurrecting deleted records would be a cleaner design pattern, the current implementation will not fail at runtime due to uniqueness violations.Likely an incorrect or invalid review comment.
.../clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java
Show resolved
Hide resolved
...anager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java:
- Around line 67-78: getValue currently queries only by name
(LocalConfigDAOImpl.getValue) and can return ambiguous results; change it to
accept a configType parameter (or add an overloaded getValue(name, configType))
and call
localPreferencesRepository.findByIsDeletedFalseAndNameAndConfigType(name,
configType) so behavior matches modifyJob/modifyConfigurations and prevents JOB
vs CONFIGURATION cross-contamination; update callers to pass the appropriate
ConfigType where necessary.
🧹 Nitpick comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
97-115: Consider usingupdate()instead ofsave()for existing records.On Line 110, when updating an existing preference, the code calls
save()(which delegates to Room'sinsertwith REPLACE strategy). While this works, it's semantically clearer and potentially more efficient to callupdate()for existing records.♻️ Proposed fix
if (existingPreference != null) { // Update existing record existingPreference.setVal(value); existingPreference.setUpdBy(RegistrationConstants.JOB_TRIGGER_POINT_USER); existingPreference.setUpdDtimes(System.currentTimeMillis()); - localPreferencesRepository.save(existingPreference); + localPreferencesRepository.update(existingPreference); } else {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalPreferencesDao.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/LocalPreferencesRepository.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java
🔇 Additional comments (5)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/LocalPreferencesRepository.java (1)
58-69: LGTM! Clean implementation with proper error handling.The new method correctly delegates to the DAO layer and includes appropriate exception handling and logging. The JavaDoc clearly explains the purpose of preventing cross-contamination between JOB and CONFIGURATION preferences.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalPreferencesDao.java (1)
28-29: LGTM! Well-formed Room query.The query correctly filters by all three criteria (is_deleted, name, and config_type) with proper parameter binding. This enables config-type scoped lookups as needed for the JOB vs CONFIGURATION segregation.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (3)
53-65: Good refactoring to consolidate update logic.The extraction of the update-or-create logic into
saveOrUpdateLocalPreferenceimproves maintainability and ensures consistent behavior across configuration and job modifications.
80-95: Good input validation and error handling.The method properly validates inputs and uses the config-type aware helper. The RuntimeException on failure ensures callers are aware of critical persistence errors.
Note: The method doesn't validate the value format (e.g., if it should be a valid cron expression). If format validation is required, consider adding it here or ensure it's performed by callers.
137-167: Excellent improvement with config-type aware cleanup.The updated logic now correctly scopes cleanup operations to CONFIGURATION type preferences only, preventing accidental deletion of JOB preferences. The added logging provides valuable observability for cleanup operations.
.../clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java:
- Around line 461-489: modifyJobCronExpression currently persists cronExpression
without validation; before calling localConfigService.modifyJob(jobId,
cronExpression) validate the cronExpression (e.g., via the existing cron
utility/parser in the project or a CronExpression validator) and if invalid
return result.error with a clear message instead of modifying storage. If valid
proceed to call localConfigService.modifyJob, then continue with fetching
SyncJobDef and rescheduling; ensure you reference the cronExpression parameter
and abort early on invalid input.
🧹 Nitpick comments (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (1)
81-96: Consider trimming the value before storage.The validation checks if
value.trim().isEmpty()but stores the originalvaluewithout trimming. This could result in storing whitespace-padded cron expressions.🔧 Proposed fix
@Override public void modifyJob(String name, String value) { if (name == null || name.trim().isEmpty()) { throw new IllegalArgumentException("Job name cannot be null or empty"); } if (value == null || value.trim().isEmpty()) { throw new IllegalArgumentException("Job value cannot be null or empty"); } try { - saveOrUpdateLocalPreference(name, value, RegistrationConstants.PERMITTED_JOB_TYPE); + saveOrUpdateLocalPreference(name.trim(), value.trim(), RegistrationConstants.PERMITTED_JOB_TYPE); } catch (Exception e) { Log.e(TAG, "Error modifying job: " + name, e); throw new RuntimeException("Failed to modify job: " + name, e); } }android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (1)
491-501: Consider: Hardcoded config type limits API flexibility.The
getValuemethod hardcodesPERMITTED_JOB_TYPE, which the comment explains is intentional for job cron expressions. However, this limits the API's reusability. If future use cases need to retrieve other config types, a separate method or an additional parameter would be required.If broader use is anticipated, consider accepting
configTypeas a parameter from the Flutter side:@Override public void getValue(@NonNull String name, @NonNull String configType, @NonNull MasterDataSyncPigeon.Result<String> result) { try { String value = localConfigService.getValue(name, configType); result.success(value); } catch (Exception e) { Log.e(TAG, "Failed to get value for: " + name, e); result.error(e); } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/app/src/main/java/io/mosip/registration_client/utils/BatchJob.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAO.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/JobManagerServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/LocalConfigService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImpl.java
- android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/JobManagerServiceImpl.java
🔇 Additional comments (11)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAO.java (1)
29-43: LGTM!The new interface methods
getValueandmodifyJobare well-documented with clear Javadoc comments explaining parameters and return values. The method signatures align with the implementation inLocalConfigDAOImpl.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/LocalConfigService.java (1)
25-45: LGTM!The new service interface methods provide a clean abstraction layer. The
getValue,modifyJob, andgetPermittedJobsmethods are well-documented and align with the DAO layer and dependent API implementations.android/app/src/main/java/io/mosip/registration_client/utils/BatchJob.java (2)
38-50: LGTM!The
LocalConfigServicedependency is properly injected via the constructor with the@Injectannotation. The field assignment follows the existing pattern in this class.
208-229: Proper fallback logic for custom cron expressions.The implementation correctly:
- Fetches the default cron from the database first
- Checks for custom cron override only if
localConfigServiceis non-null- Uses
syncJob.getId()(not the API name) to retrieve the custom value, which aligns with howmodifyJobstores values by job IDThe null and empty string checks on
customCronat line 220 prevent accidental blank overrides.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dao/LocalConfigDAOImpl.java (3)
67-79: LGTM!The
getValueimplementation correctly uses configType-aware lookup and handles exceptions gracefully by logging and returning null. This aligns with the method contract documented in the interface.
98-116: LGTM!The
saveOrUpdateLocalPreferencehelper method properly consolidates the update-or-create logic with configType awareness. The Javadoc correctly documents the prevention of cross-contamination between JOB and CONFIGURATION preferences.
138-168: Clean-up logic correctly scoped to CONFIGURATION type.The updated
cleanUpLocalPreferencesmethod now uses configType-aware lookup (line 151-152), ensuring that onlyPERMITTED_CONFIG_TYPEpreferences are cleaned up. This prevents accidental deletion of JOB-type preferences during cleanup operations.android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java (4)
105-152: LGTM!The
LocalConfigServicedependency is properly injected and assigned, following the existing pattern for other dependencies in this class.
439-448: LGTM!The
getPermittedJobsmethod properly handles null by returning an empty list, preventing NPEs on the Flutter side.
450-459: LGTM!The
isValidCronExpressionmethod delegates toCronExpressionParserand returnsfalseon exceptions rather than propagating errors. This is appropriate for a validation endpoint.
587-597: LGTM!The
getJobIdByApiNamemethod now uses the repository'sgetSyncJobDefByApiNamemethod for a cleaner lookup. The fallback to an empty string on error is consistent with existing behavior.
android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java
Show resolved
Hide resolved
3 participants
RCF-1278
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.