Skip to content

[MBL-19568][Student] Fix folder bookmarking and quiz routing #3430

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.

Sign up for GitHub

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

Closed
tamaskozmer wants to merge 1 commit into from
Closed

[MBL-19568][Student] Fix folder bookmarking and quiz routing #3430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/student/src/main/java/com/instructure/student/features/files/list/FileListFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ class FileListFragment : ParentFragment(), Bookmarkable, FileUploadDialogParent
}

override val bookmark: Bookmarker
get() = Bookmarker(canvasContext.isCourseOrGroup, canvasContext)
get() = Bookmarker(canvasContext.isCourseOrGroup && folder?.isRoot == true, canvasContext)
Copy link

@claude claude bot Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private fun animateFabs() = with(binding) {
if (mFabOpen) {
Expand Down
7 changes: 7 additions & 0 deletions apps/student/src/main/java/com/instructure/student/router/RouteMatcher.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,13 @@ object RouteMatcher : BaseRouteMatcher() {
AssignmentDetailsFragment::class.java
)
)
routes.add(
Route(
courseOrGroup("/:${RouterParams.COURSE_ID}/assignments/:${RouterParams.ASSIGNMENT_ID}"),
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.
Copy link

@claude claude bot Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. How does the router handle duplicate patterns? Does it use the first match or the last match?
  2. Will this cause any issues with route matching or navigation?
  3. 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.


// Studio
routes.add(
Expand Down
Loading