[MBL-19568][Student] Fix folder bookmarking and quiz routing #3430
Conversation
- Disable bookmarking for non-root folders to prevent routing issues with embedded links - Add route for new quizzes opened from quiz list to Assignment Details Test Plan: - Verify bookmarking is only available in root folder - Verify new quizzes route to Assignment Details correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Thank you for this PR! I've reviewed the changes related to bookmarking behavior and quiz routing. Here's my assessment:
Changes Overview
- FileListFragment.kt: Modified bookmark logic to only enable bookmarking when in a course/group AND at the root folder
- RouteMatcher.kt: Added a new route for assignments to support new quizzes navigation from the quiz list
Issues Found
-
FileListFragment.kt:603 - Null safety consideration: Verify the intended behavior when
folderis null. Currentlyfolder?.isRoot == truewill returnfalsewhen folder is null, disabling bookmarks. Confirm this is the desired behavior and consider adding a clarifying comment. -
RouteMatcher.kt:408 - Duplicate route pattern detected: The same URL pattern
courseOrGroup("/:COURSE_ID/assignments/:ASSIGNMENT_ID")is registered twice with different previous fragments. This may cause routing ambiguity depending on how the router handles duplicates. Please verify this won't cause navigation issues and consider whether consolidation is needed.
Positive Feedback
- The code changes are concise and focused
- Good inline comment explaining the purpose of the new route for new quizzes
- The bookmark logic change appears to be a targeted fix for a specific use case
Recommendations
- Add unit tests for the modified bookmark logic with various folder states (null, root, non-root)
- Verify the router's behavior with duplicate route patterns through testing
- Consider adding a code comment in FileListFragment explaining why only root folders should be bookmarkable
Test Coverage
Please ensure there are tests covering:
- Bookmark availability with null folder
- Bookmark availability with root folder
- Bookmark availability with non-root folder
- Navigation from quiz list to assignment details using the new route
Let me know if you have any questions about this feedback!
|
|
||
| override val bookmark: Bookmarker | ||
| get() = Bookmarker(canvasContext.isCourseOrGroup, canvasContext) | ||
| get() = Bookmarker(canvasContext.isCourseOrGroup && folder?.isRoot == true, canvasContext) |
There was a problem hiding this comment.
Potential NPE concern: The expression folder?.isRoot == true uses safe navigation, but consider what happens if folder is null. When folder is null, folder?.isRoot evaluates to null, and null == true evaluates to false. This means the bookmark will not be enabled for null folders.
Is this the intended behavior? If folder being null should also disable bookmarking, this is correct. However, if there are scenarios where folder could be null but bookmarking should still be allowed (e.g., at the root level before folder is initialized), this might cause issues.
Consider adding a comment explaining the expected behavior when folder is null.
| QuizListFragment::class.java, | ||
| AssignmentDetailsFragment::class.java | ||
| ) | ||
| ) // Route for new quizzes opened from the quiz list. New quizzes are shown on the Assignment details. This is needed for the bookmarker to find the correct route. |
There was a problem hiding this comment.
Duplicate route pattern: This new route uses the same URL pattern as the route added just above it (lines 401-405):
- Line 402:
courseOrGroup("/:${RouterParams.COURSE_ID}/assignments/:${RouterParams.ASSIGNMENT_ID}") - Line 408:
courseOrGroup("/:${RouterParams.COURSE_ID}/assignments/:${RouterParams.ASSIGNMENT_ID}")
Both routes point to AssignmentDetailsFragment::class.java as the target fragment, but have different previous fragments (QuizListFragment vs the default).
Questions:
- How does the router handle duplicate patterns? Does it use the first match or the last match?
- Will this cause any issues with route matching or navigation?
- Should these be consolidated into a single route with conditional logic, or is the duplicate intentional for handling different navigation contexts?
The comment explains the intent, but the implementation might cause routing ambiguity.
📊 Code Coverage Report
|
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Thu, 04 Dec 2025 14:47:57 GMT |
|
Student Install Page |
Test Plan
Folder Bookmarking
Quiz Routing
refs: MBL-19568
affects: Student
Release Note
Fixed folder bookmarking to prevent issues with embedded links in non-root folders
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]