fix: Android- Remove "Leaving the app" popup on program screen ... - #214
fix: Android- Remove "Leaving the app" popup on program screen ...#214kalalmounesh wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Discovery WebView navigation so in-app pages (course/program/discovery) don’t trigger the “Leaving the app” dialog when users follow links between trusted Open edX domains, aligning with LEARNER-11069.
Changes:
- Treat links as internal by default on Program and Course info screens (
isAllLinksExternal = false) and rely on host-based logic to decide when to show the external-link dialog. - Add trusted/always-external host allowlists to the Discovery WebView client, derived from configured app URLs.
- Improve session-cookie refresh by awaiting WebView cookie removal/setting, and wire
AppCookieManagerintoCourseInfoViewModel.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| discovery/src/main/java/org/openedx/discovery/presentation/program/ProgramFragment.kt | Stops forcing all links external on program WebView and removes manual loadUrl update hook. |
| discovery/src/main/java/org/openedx/discovery/presentation/info/CourseInfoViewModel.kt | Injects/exposes AppCookieManager for WebView cookie refresh. |
| discovery/src/main/java/org/openedx/discovery/presentation/info/CourseInfoFragment.kt | Passes cookie manager into WebView and stops forcing all links external; adds handling for enrolled link authorities. |
| discovery/src/main/java/org/openedx/discovery/presentation/catalog/DefaultWebViewClient.kt | Adds trusted/always-external host logic to suppress “Leaving the app” for trusted domains; adjusts override behavior. |
| discovery/src/main/java/org/openedx/discovery/presentation/catalog/CatalogWebView.kt | Builds trusted host sets from Config and supplies them to DefaultWebViewClient. |
| core/src/main/java/org/openedx/core/system/AppCookieManager.kt | Makes cookie removal/setting awaitable and sets cookies for multiple configured targets. |
| app/src/main/java/org/openedx/app/di/ScreenModule.kt | Updates Koin wiring to provide the new AppCookieManager dependency to CourseInfoViewModel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val trustedBase = trustedHost.split(".").takeLast(2).joinToString(".") | ||
| host == trustedHost || host == trustedBase || host.endsWith(".$trustedBase") |
There was a problem hiding this comment.
This comment is also already addressed.
| val cookieTargets = buildSet { | ||
| add(config.getApiHostURL()) | ||
| add(config.getDiscoveryConfig().webViewConfig.baseUrl) | ||
| add(config.getProgramConfig().webViewConfig.programUrl) | ||
| } |
There was a problem hiding this comment.
This comment is also already addressed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
discovery/src/main/java/org/openedx/discovery/presentation/catalog/DefaultWebViewClient.kt:95
- In
onReceivedHttpError, the retry doesrefreshSessionCookie()and immediately callswebView.loadUrl(...). In current call sites,refreshSessionCookielaunches a coroutine (async), so the reload will typically happen before cookies are refreshed, andhasRetriedprevents any further retry—potentially leaving the WebView stuck on 401/403. Consider changing the contract so the reload happens after the cookie refresh completes (e.g., haverefreshSessionCookieaccept a completion callback / URL to reload, or perform the reload from inside the provided lambda once refresh finishes).
if (request.url.toString() == view.url && !hasRetried) {
when (errorResponse.statusCode) {
403, 401 -> {
hasRetried = true
refreshSessionCookie()
webView.loadUrl(request.url.toString())
}
}
| viewModel.infoCardClicked( | ||
| fragmentManager = requireActivity().supportFragmentManager, | ||
| pathId = param, | ||
| infoType = type.name |
| private var hostForThisPage: String? = null | ||
| private var isPossibleRedirection = true | ||
| private var hasRetried = false | ||
| private var hasPendingUserNavigation = false |
There was a problem hiding this comment.
Inconsistent State Management The flags hasPendingUserNavigation and hasRetried are not thread-safe. If multiple URLs load simultaneously, there could be race conditions.
LEARNER-11069