sync: smoother configuration coordinating (fixes #11706)#11651
Conversation
…cess to SyncConfigurationCoordinator Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
❌ 3 blocking issues (3 total)
|
app/src/main/java/org/ole/planet/myplanet/ui/sync/SyncConfigurationCoordinator.kt
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
| } |
Okuro3499
left a comment
There was a problem hiding this comment.
1. Lazy initialization is fragile (must-fix)
syncConfigurationCoordinator is lateinit var and initialized inside a if (!::syncConfigurationCoordinator.isInitialized) guard on first call to checkMinApk. There is no reason this can't be initialized in onCreate. If checkMinApk is called concurrently from multiple call sites before the first call completes (possible given it's triggered from
LoginActivity, DashboardElementActivity, and two paths in ServerDialogExtensions), the guard is not thread-safe and initialization can run twice. Initialize in onCreate and remove the guard entirely.
2. Getter callbacks invert the dependency (must-fix)
Three interface methods — getServerConfigAction(), getCurrentDialog(), getServerDialogBinding() — pull volatile state from the activity into the coordinator at call time. This means the coordinator is not a self-contained unit; it depends on querying the activity for state mid-execution. The cleaner pattern is to pass these values as parameters to checkMinApk, or as constructor arguments if they're stable. As-is, the coordinator is tightly coupled to the activity's internal state machine without making that coupling explicit.
3. onSaveConfigAndContinue callback signature is misleading (nice-to-have)
The interface declares url: String and isAlternativeUrl: Boolean parameters, but the coordinator always calls it with hardcoded "" and false (SyncConfigurationCoordinator.kt:78). The parameters are dead in practice. Either remove them from the interface and pass defaultUrl only, or document why they exist.
4. callerActivity is a stringly-typed enum (nice-to-have)
Routing on "LoginActivity" / "DashboardActivity" string literals was a pre-existing smell; the extraction is a good opportunity to introduce a sealed class or enum for CallerContext. As-is, a typo silently falls through to the else branch.
5. No lifecycle safety for the in-flight coroutine
checkMinApk receives lifecycleScope as a parameter, which is correct. But dismissProgressDialog() is called synchronously after the getMinApk suspension point — if the activity is destroyed while the request is in-flight (e.g., user rotates), the callback fires into a dead activity. This was equally present before the PR, so it's not a regression, but the extraction is a natural place to note it.
…cess to SyncConfigurationCoordinator Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Extracted the version checking (
checkMinApk) and server configuration handling logic (handleConfigurationSuccess) fromSyncActivityinto a newSyncConfigurationCoordinatorclass. The original signature ofcheckMinApkwas kept the same, as requested. The UI-related actions like displaying progress dialogs or error popups were handled via aCallbackinterface implemented inSyncActivity.PR created automatically by Jules for task 15088535178337642235 started by @dogi