GT-3014 Account for hidden pages in lesson share links & deep links - #4568
GT-3014 Account for hidden pages in lesson share links & deep links#4568tjohnson009 wants to merge 2 commits into
Conversation
The lesson pager only contains visible pages, so the pager index of the current page doesn't account for any hidden pages before it. iOS and knowgod.com use the page position within the full list of manifest pages, so share links generated on Android pointed at the wrong page whenever a lesson contains hidden pages. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lesson share links generated by iOS and knowgod.com reference the page position within the full list of manifest pages, but Android was using the number as an index into the visible pages of the lesson pager, landing on the wrong page when a lesson contains hidden pages. Hidden pages resolve to the closest previous visible page (matching the lesson resume behavior) since hidden pages are only revealed by content events, and invalid positions resolve to the first page. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4568 +/- ##
===========================================
+ Coverage 52.84% 52.86% +0.01%
===========================================
Files 448 449 +1
Lines 12074 12081 +7
Branches 2089 2091 +2
===========================================
+ Hits 6381 6387 +6
- Misses 5064 5066 +2
+ Partials 629 628 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| snapshotFlow { pagerState.settledPage }.collect { page -> | ||
| snapshotFlow { pagerState.settledPage to lessonPagerState.pages }.collect { (page, pages) -> | ||
| dataModel.pageReached.update { maxOf(it, page) } | ||
| dataModel.currentPage.value = page | ||
| dataModel.currentPagePosition.value = pages.getOrNull(page)?.position ?: 0 |
There was a problem hiding this comment.
a better way to handle this is to probably expose a settledPage: Page property on the LessonPagerState object that is derived from from pages[pagerState.settledPage].
I'm not fully sure if it needs to be nullable or not, pagerState is tightly coupled to the rest of LessonPagerState, so it should be capped to exactly the number of pages available in LessonPagerState.pages. Which means that .getOrNull() may not be necessary to protect against an IndexOutOfBoundsException.
You may also want to separate the highest page reached logic from the logic to determine the current position into 2 separate LaunchedEffects to keep the 2 flows simple and not have to rely on combining multiple values in a Pair
There was a problem hiding this comment.
I've gone back and forth with Claude on this and I'm not sure either. But I think the null check feels like a very small price to pay to avoid a potential crash. I asked Claude if there was ever a scenario where a lesson has no pages, because that doesn't seem like a possibility fro what I can tell and it listed a few, but I don't know how realistic it sounds.
There was a problem hiding this comment.
I'll get on the other things first, and then let me know where you land the plane on the nullable part. I think it is a small price to pay, but I might not be seeing this fully
There was a problem hiding this comment.
I didn't post a followup, but thinking about it more we do want to keep it nullable because of the empty pages scenario.
I'm about to open a PR on godtools-shared with a settledPage: LessonPage? property you can use
There was a problem hiding this comment.
Okay cool. I was mid-flight on doing that, but I'll tell Claude to pause and then look for it in a bit.
There was a problem hiding this comment.
2 participants
Fixes GT-3014
Problem
The lesson pager only contains visible pages, but iOS and knowgod.com reference pages by their position within the full list of manifest pages (including hidden pages). Android was using the pager index in both directions, so whenever a lesson contains hidden pages (e.g. the Listening Lesson in English goes 0 → 1 → 5):
Changes
Share links (
GT-3014 Build the lesson share link from the current page position)positionwithin the full list of manifest pages (currentPagePosition) instead of the pager index, and the share link is built from that. The tracker also observes the pager's visible pages, so the share link stays correct if a hidden page is revealed mid-lesson.pageReached(lesson feedback) intentionally keeps its existing pager-index semantics.Deep links (
GT-3014 Resolve lesson deep link page numbers as manifest page positions)Testing
Manifest.lessonPagerIndexForPagePosition()covering visible pages, hidden pages, invalid positions, and a hidden first page./gradlew :ui:lesson-renderer:testand ktlint pass on each commitNotes
LessonPagerStatere-hides them when they're not the current page. If that's ever wanted (per the open question on the ticket), it would need a shared-library change.🤖 Generated with Claude Code