-
Notifications
You must be signed in to change notification settings - Fork 108
[MBL-19631][All] Prevent external tool launch errors when offline #3466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
This PR adds offline detection for LTI (Learning Tools Interoperability) external tools, preventing users from attempting to launch external tools when there's no network connection. Overall approach is solid, but there are several issues to address.
Issues Found
-
Context lifecycle issues with Toast display - Multiple locations use
ContextKeeper.appContextto show Toasts, which can cause display issues since application context isn't tied to an activity window. See comments inBindingAdapters.kt:200,WebViewExtensions.kt:87, andCanvasWebView.kt:889 -
Code style inconsistency - Fully qualified
android.widget.Toastused in some files instead of imports. Per CLAUDE.md: "Always use imports instead of fully qualified names in code". SeeWebViewExtensions.kt:87andCanvasWebView.kt:889 -
Network detection could be more robust - The updated
isNetworkAvailable()only checksNET_CAPABILITY_INTERNET, which indicates capability but not actual connectivity. Consider addingNET_CAPABILITY_VALIDATEDcheck for more accurate offline detection. SeeUtils.kt:53 -
Code duplication - The offline check logic is duplicated across 4 different files. Consider extracting to a shared helper method or handling this at a single entry point
Positive Aspects
✓ Good modernization of the network detection API from deprecated activeNetworkInfo to NetworkCapabilities
✓ Consistent user-facing error message using string resource
✓ Appropriate use of early return pattern for readability
✓ Covers multiple entry points for LTI tool launches
Recommendations
- Fix Toast context issues - Either use the Fragment's context in
DiscussionDetailsFragment.kt(where it's available viarequireContext()), or consider an event-based approach where the UI layer handles user feedback - Add proper imports - Replace fully qualified class names with imports
- Enhance network detection - Add
NET_CAPABILITY_VALIDATEDcheck for more accurate connectivity detection - Consider refactoring - The duplication suggests this logic might be better centralized, perhaps in a wrapper around the LTI callback
Test Coverage
No tests were added in this PR. Consider adding:
- Unit tests for the updated
Utils.isNetworkAvailable()method - UI tests verifying the Toast appears when offline and LTI tools are blocked
libs/pandautils/src/main/java/com/instructure/pandautils/binding/BindingAdapters.kt
Outdated
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/utils/WebViewExtensions.kt
Outdated
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/views/CanvasWebView.kt
Outdated
Show resolved
Hide resolved
🧪 Unit Test Results✅ 📱 Parent App
✅ 📱 Student App
✅ 📱 Teacher App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Tue, 06 Jan 2026 15:28:11 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
|
kdeakinstructure
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA 👍
adamNagy56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA +1
Summary
Prevents errors when users attempt to launch external LTI tools while offline by adding network connectivity checks and displaying an informative toast message.
Test plan
Changes
Utils.isNetworkAvailable()to use modern NetworkCapabilities API instead of deprecated activeNetworkInfoltiToolsOfflinefor the error messagerefs: MBL-19631
affects: Student, Teacher, Parent
release note: Fixed errors when attempting to launch external tools while offline
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com