[MBL-19497][Student] Fix bookmark URL placeholders from notifications #3417
Conversation
Fixed issue where bookmarking assignments opened from notifications or ToDo list generated URLs with placeholder segments like :sliding_tab_type and :submission_id, making the bookmarks unusable. The fix adds regex-based placeholder stripping in BookmarkCreationDialog after URL generation, ensuring clean URLs without affecting route matching or existing routing logic. Changes: - Added placeholder stripping logic to BookmarkCreationDialog - Strips segments matching /:[^/?#]+ pattern from generated URLs - Applied to both single fragment and dual fragment bookmark flows Test plan: - Navigate from notifications to assignment details and bookmark - Verify bookmark URL is clean without placeholders - Test bookmarking from assignments list, calendar, and ToDo list - Verify all bookmark URLs are properly formed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
kristofnemere
changed the title
![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 URL cleanup logic to strip unreplaced placeholder segments from bookmark URLs. The fix addresses a valid issue where RouteMatcher.generateUrl() may leave placeholder segments (like /:sliding_tab_type/:submission_id) in URLs when replacement parameters aren't provided.
Positive Feedback
✅ Good problem identification: The fix addresses a real issue where URL generation can leave placeholder segments unreplaced
✅ Strategic placement: The cleanup is applied after URL generation attempts, which is the right approach
✅ Clear comments: The inline comments explain why this cleanup is needed
Issues Found
- Code duplication (lines 166-168 and 185-187): The placeholder stripping logic is duplicated. Extract to a private helper function to follow DRY principles and improve maintainability -
BookmarkCreationDialog.kt:166 - Regex pattern could be more precise: The pattern
/:[^/?#]+might match edge cases beyond typical placeholders. Consider using/:([a-z_]+)to match only lowercase placeholder-style segments -BookmarkCreationDialog.kt:168
Additional Considerations
Missing test coverage: This file has no unit tests. Consider adding tests to verify:
- Placeholder segments are properly stripped
- Valid URL segments are preserved
- Edge cases (empty URLs, URLs with query params, etc.)
Root cause: While this fix works as a bandaid, the underlying issue is that RouteMatcher.generateUrl() returns URLs with unreplaced placeholders. Consider whether the root cause should be addressed in Route.createUrl() (at Route.kt:279) to either throw an error or strip placeholders when replacement params are missing.
Code Quality
The implementation follows the project's Kotlin conventions and integrates well with the existing code structure. The inline comments are helpful and the fix is minimally invasive.
apps/student/src/main/java/com/instructure/student/dialog/BookmarkCreationDialog.kt
Show resolved
Hide resolved
📊 Code Coverage Report✅ Student
✅ Teacher
|
|
Student Install Page |
adamNagy56
left a comment
adamNagy56
left a comment
There was a problem hiding this comment.
- In the Files menu, when I try to bookmark a folder, the button doesn’t do anything, it’s not possible to bookmark it.
- When I open a New Quiz type from the Quizzes menu, I also can’t bookmark it because the button doesn’t respond there either.
See attached video.
Screen_Recording_20251128_130018_Canvas.mp4
kdeakinstructure
left a comment
kdeakinstructure
left a comment
There was a problem hiding this comment.
QA 👍
The found issues (subfolder bookmark, new quiz bookmark) are existing production issues, not exactly related to this bugfix. However, decide if you fix them in the scope of this PR, I can accept if not, but then please make separate bug ticket(s) for them.
Test Plan
IMPORTANT: Please smoke test bookmarking functionality since we modified the bookmark URL generation logic.
Primary Test Cases
From Notifications List:
From ToDo List:
From Assignments List (baseline):
From Calendar:
Additional Smoke Tests for Bookmarking
Since this PR modifies bookmark URL generation:
:sliding_tab_typeor:submission_idExpected Behavior
https://domain.com/courses/123/assignments/456https://domain.com/courses/123/assignments/456/:sliding_tab_type/:submission_idrefs: MBL-19497
affects: Student
release note: Fixed an issue where bookmarks created from notifications or the ToDo list would fail to open