Conversation
🧪 Unit Test Results📊 Summary
Last updated: Fri, 06 Feb 2026 07:37:18 GMT |
![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 adds LTI routing improvements and excludes certain E2E tests from release builds. Overall the changes look reasonable, but there are some concerns to address.
Issues Found
- Debug logging in production code (RouteMatcher.kt:332) - Remove or conditionally compile debug log statement
- Duplicated sessionless_launch logic (RouteMatcher.kt:491 and LtiLaunchFragment.kt:180) - Extract to shared utility for consistency
- Fragile URL string matching (LtiLaunchViewModel.kt:125) - Use proper URL parsing instead of substring matching for session_token detection
- Missing error logging (RouteMatcher.kt:506) - Add logging when falling back to handleWebViewUrl for debugging
- Potential null safety issue (RouteMatcher.kt:496-506) - Consider early return when contextId cannot be determined
Positive Aspects
✅ Good addition of @ReleaseExclude annotation to prevent problematic E2E tests from running in release builds
✅ Version bump is properly applied (versionCode 88, versionName 2.4.0)
✅ The LTI routing logic follows a clear separation of concerns
✅ The sessionless_launch URL handling prevents infinite routing loops, which is a good defensive practice
Recommendations
- Add test coverage: The new LTI routing logic in
handleLtiRoute()needs unit tests, especially for edge cases (missing context, invalid URLs, etc.) - Document the sessionless_launch behavior: Add comments explaining why these URLs are special and need to be excluded from internal routing
- Consider URL parsing utilities: Replace string contains checks with proper URL parsing (e.g., using
Uri.parse()and checking query parameters) - Review logging strategy: Ensure debug logs are either removed or wrapped in build config checks for production
The core functionality looks solid, but addressing the code quality and maintainability issues will help prevent future bugs.
| @@ -329,6 +330,7 @@ object RouteMatcher : BaseRouteMatcher() { | |||
|
|
|||
| fun route(activity: FragmentActivity, route: Route?) { | |||
| val uri = route?.uri | |||
There was a problem hiding this comment.
Consider removing this debug logging statement before merging to production. Debug logs with detailed routing information can clutter logs and potentially expose sensitive information in production builds.
| return | ||
| } | ||
|
|
||
| // Don't intercept sessionless_launch URLs - they're already being handled by an existing LtiLaunchFragment |
There was a problem hiding this comment.
The comment says "Don't intercept sessionless_launch URLs" but this check might be too broad. Consider whether this should be more specific (e.g., checking for the full path pattern rather than just substring matching). Also, this logic appears duplicated in LtiLaunchFragment.kt:180 - consider extracting to a shared utility function to maintain consistency.
|
|
||
| val contextId = route.paramsHash[RouterParams.COURSE_ID]?.toLongOrNull() | ||
| ?: route.paramsHash[RouterParams.GROUP_ID]?.toLongOrNull() | ||
|
|
||
| val canvasContext: CanvasContext? = when (route.getContextType()) { | ||
| CanvasContext.Type.COURSE -> contextId?.let { Course(id = it) } | ||
| CanvasContext.Type.GROUP -> contextId?.let { Group(id = it) } | ||
| else -> null | ||
| } | ||
|
|
||
| if (canvasContext != null) { |
There was a problem hiding this comment.
There's a potential null safety issue here. If contextId is null and both conditions fail, the function continues to check canvasContext != null, which will always be null in that case. Consider early return or better error handling when the context cannot be determined. Also, consider logging when falling back to handleWebViewUrl so debugging is easier.
| private fun launchLti(url: String) { | ||
| viewModelScope.launch { | ||
| val authenticatedUrl = repository.authenticateUrl(url) | ||
| val authenticatedUrl = if (url.contains("session_token")) url else repository.authenticateUrl(url) |
There was a problem hiding this comment.
This conditional authentication logic could be problematic. The check if (url.contains("session_token")) is a simple substring match which could produce false positives. Consider:
- Using a more robust URL parsing approach (checking query parameters specifically)
- Adding a comment explaining why URLs with session_token should skip authentication
- Adding test coverage for this behavior to prevent regressions
| private fun canRouteInternally(url: String) = | ||
| webViewRouter.canRouteInternally(url) && ltiUrl?.substringBefore("?") != url.substringBefore("?") | ||
| webViewRouter.canRouteInternally(url) | ||
| && ltiUrl?.substringBefore("?") != url.substringBefore("?") |
There was a problem hiding this comment.
Same sessionless_launch check as in RouteMatcher.kt:491. This duplication suggests these classes are making routing decisions independently. Consider:
- Extracting the sessionless_launch detection to a shared utility
- Documenting why sessionless_launch URLs need special handling
- Ensuring both checks remain synchronized if the logic needs to change
📊 Code Coverage Report✅ Student
|
2 participants
No description provided.