[CLX-3342][Horizon]Report a bug change #3400
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR successfully simplifies the "Report a Bug" functionality by removing the complex WebView-based implementation and replacing it with a direct external URL link. This is a great improvement that reduces code complexity and maintenance burden.
Positive Changes ✅
- Reduced complexity: Removed 147 lines of complex WebView code including JavaScript interface handling, file picker callbacks, and video client setup
- Cleaner architecture: Eliminated unnecessary navigation route and dedicated screen for a simple external link
- Better user experience: Opens the bug report URL directly in the user's browser instead of an embedded WebView
- Improved maintainability: Simpler code with fewer moving parts and dependencies
Issues to Address
- Hardcoded URL (libs/horizon/src/main/java/com/instructure/horizon/features/account/AccountViewModel.kt:122): The bug report URL is hardcoded in the ViewModel. Consider extracting it to a constant for better maintainability
- Missing error handling (libs/horizon/src/main/java/com/instructure/horizon/features/account/AccountScreen.kt:171): The
uriHandler.openUri()call should be wrapped in try-catch to handle cases where no browser is available
Code Quality
- Architecture follows MVVM pattern correctly
- Changes are focused and well-scoped
- Removed unused code completely (no backwards-compatibility hacks)
- Type changes are consistent across the codebase
Testing Recommendations
- Verify the external URL opens correctly in the default browser
- Test behavior when no browser app is installed on the device
- Ensure the account screen still renders correctly with the simplified implementation
Overall, this is a solid refactoring that improves the codebase. The suggested improvements are minor and would further enhance robustness.
| AccountItemState( | ||
| title = context.getString(R.string.accountReportABug), | ||
| type = AccountItemType.OpenExternal(AccountRoute.BugReportWebView) | ||
| type = AccountItemType.OpenExternal("https://community.canvaslms.com/t5/Canvas-Career/Report-an-issue/td-p/662564") |
There was a problem hiding this comment.
Consider extracting this URL to a constant or configuration file instead of hardcoding it in the ViewModel. This would make it easier to update the URL in the future and improve testability.
companion object {
private const val BUG_REPORT_URL = "https://community.canvaslms.com/t5/Canvas-Career/Report-an-issue/td-p/662564"
}|
|
||
| is AccountItemType.OpenExternal -> navController.navigate(item.type.route.route) | ||
| is AccountItemType.OpenExternal -> { | ||
| uriHandler.openUri(item.type.url) |
There was a problem hiding this comment.
Good use of LocalUriHandler for opening external URLs! However, consider adding error handling in case the URI cannot be opened (e.g., no browser installed):
is AccountItemType.OpenExternal -> {
try {
uriHandler.openUri(item.type.url)
} catch (e: Exception) {
// Log error or show a toast to the user
}
}
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
|
Student Install Page |
andrasmaczak
left a comment
andrasmaczak
left a comment
There was a problem hiding this comment.
Don't merge it yet, the url might be changed
refs: CLX-3342
affects: Student
release note: none