feat(ios): drawer/source switcher and shell navigation parity — shared NavigationDrawer, real back stack - #1153
Merged
Merged
Conversation
…/es/bg) Adds compose.components.resources dependency and copyComposeResourcesForApk task to :feature:library-ui, creates the nine drawer/shell strings in values/strings.xml with Spanish and Bulgarian translations, and wires the asset bridge into :app so Android picks them up at runtime.
…drawer for Android + iOS Moves RiffleNavigationDrawer and helpers from :app into feature/library-ui (android+ios topology). Replaces androidx.compose.material.icons with RiffleIcons from feature/design-system (adds Download, Settings, KeyboardArrowDown, KeyboardArrowUp). Adds appVersion/appSha parameters so each host provides its own version string. Fixes all downstream import sites in :app (MainScreen, RiffleScreen, test files).
Replace the iOS-only DrawerViewModel with the shared NavigationDrawerViewModel from :feature:navigation, and replace the inline DrawerSheetContent in shared/HomeScreen.kt with the shared RiffleNavigationDrawer from :feature:library-ui. - Delete shared/DrawerViewModel.kt (behaviour covered by NavigationDrawerViewModel) - Delete shared/DrawerViewModelTest.kt (behaviour covered by NavigationDrawerViewModelTest) - Rewrite HomeScreen.kt: use NavigationDrawerViewModel, collect isRiffleMode / showDownloadsLink / serverVersions, call RiffleNavigationDrawer - Fix shell states: CircularProgressIndicator for null destination; error+Retry for NoLibraries - Register NowPlayingNavigator + NavigationDrawerViewModel in iOS Koin module - Add :feature:navigation dependency to :shared Removed-test: allServersFiltersOutStorytellerService Removed-test: activeServerIsTheFirstActiveSource Removed-test: activeServerIsNullWhenNoSourceIsActive Removed-test: visibleLibrariesFiltersHiddenIds Removed-test: setRiffleActiveClearsActiveSource Removed-test: setRiffleActiveSetsRiffleFlag Removed-test: visibleLibrariesFiltersReadaloudLibraries
…library-ui commonTest Port 9 subtitle/overflow-font tests from app/src/test (JUnit4) to feature/library-ui/src/commonTest (kotlin.test), so they run on both Android and iOS simulator. Updated assertion arg order to message-last per kotlin.test convention.
Replace the single mutableStateOf<LibraryNav> with a stack (List<LibraryNav>) so that back navigation returns to the previous screen rather than always jumping to the top-level library list. Gaps fixed: - Detail → back returns to the list/series/collection it came from - Reader → back returns to the item detail sheet - Filtered books / annotation search / section → back returns to caller rememberSaveable is intentionally not used because LibraryNav.ReaderDestination carries a LibraryItem that is not Parcelable/Serializable; the stack resets on process death, which is acceptable.
Use LocalWindowInfo.containerSize to derive the window-width and height buckets, mirroring the Android isTabletLayout() predicate (≥840dp wide, ≥480dp tall). When the layout qualifies as tablet: - pass usePermanentDrawer=true so RiffleNavigationDrawer renders a persistent side-rail instead of a modal overlay - hide the drawer panel in reader routes (hidePermanentDrawerPanel) via a SideEffect + DisposableEffect on LibraryHost's navStack
…footer) ND-5: The source switcher is now a DropdownMenu rather than inline expanded text. Updated the test to verify the header shows the source name once when collapsed and at least twice (header + dropdown item) after tapping. ND-8: Added version footer test — iOS passes appVersion=null so RiffleNavigationDrawer suppresses the footer; assert no "Riffle v*" text appears in the open drawer. Also update the header comment to reference the test that moved to feature/library-ui/src/commonTest.
…migrate DrawerSheetContentTest to RiffleNavigationDrawer - Remove ui_drawer_riffle (translatable=false) from values-es and values-bg — checkTranslations rejects translatable=false strings in locale-specific files - Update DrawerSheetContentTest to call RiffleNavigationDrawer (with usePermanentDrawer=true so the panel is always visible) instead of the deleted DrawerSheetContent composable; behavioral claims (library list hidden when isRiffleActive, shown otherwise) are preserved with identical test names
…ed ListItem semantics
Material3 ListItem with Modifier.clickable sets mergeDescendants=true in the CMP semantic
tree. All child texts (source name, username, host, arrow icon) collapse into the parent
element's single merged accessibility label; individual children are not exposed as
separate StaticText nodes in XCUITest. The previous test approach of staticTexts["Audiobookshelf"]
failed because the source name was inside this merged element.
Changes:
- Add NAV_DRAWER_SOURCE_HEADER constant to TestTags.kt (maintains testTag convention for
Compose test rules; does not help XCUITest due to merged semantics)
- Add .testTag(TestTags.NAV_DRAWER_SOURCE_HEADER) to the ListItem in DrawerHeader
- Update NavDrawerTests.swift:
- sourceHeaderButton() now uses NSPredicate(label CONTAINS[c] 'Audiobookshelf') across
all element types (buttons, cells, otherElements) to find the merged header element
- ND-3, ND-4: verify header exists and label contains "Audiobookshelf" / host
- ND-5: count-based check uses descendants(matching: .any) + CONTAINS predicate;
dropdown-open check uses CONTAINS predicate excluding the header by index-0
- ND-7 (renamed to testDownloadsIsAbsentForAbsOnlySourceOnIos): assert Downloads is
ABSENT because AbsCatalog (JVM-only) provides no DownloadsCapability on iOS, so
NavigationDrawerViewModel sets showDownloadsLink=false for ABS-only setups
- ND-8: wait for "Settings" as the drawer-open signal instead of "Audiobookshelf"
…I visibility - Replace hardcoded "Unable to connect to source" / "Retry" in HomeScreen.kt with stringResource(Res.string.ui_unable_to_connect_to_source/ui_retry) so Spanish and Bulgarian users see translated text instead of English - Fix LaunchedEffect key: use drawerViewModel (stable ViewModel instance) instead of drawerViewModel.redirectToLibrary (Flow object) — semantically equivalent today but correct practice when ViewModel identity is the restart signal - Mark nextOverflowFontSize, buildSupportingLine, sourceSwitcherSubtitle as internal in NavigationDrawerComposable.kt — they are only used within feature:library-ui's own commonTest and have no external consumers; only sourceDisplayName stays public because RiffleScreen.kt in :app imports it
…ce drawer assertion ktlint violations in NavigationDrawerComposable.kt: - Import ordering: moved DrawerState above DropdownMenu (lexicographic), moved testTag import to correct position within androidx.compose.ui.platform block, moved org.jetbrains.compose.resources.stringResource before alias imports so aliased imports appear last per ktlint's "aliases in the end" rule - Missing braces: expanded if/else on supportingContent and sourceDisplayName / localizedSourceDisplayName into multi-line block form AddAbsSourceFlowTests regression fix: - Line 181: staticTexts["Audiobookshelf"] fails after our change because the drawer source-switcher header is a Material3 ListItem with mergeDescendants=true — the source name is merged into the parent button's label, not exposed as a separate StaticText. Updated to use NSPredicate(label CONTAINS[c] 'Audiobookshelf') across all descendants, matching the pattern established in NavDrawerTests.swift.
…tag assertions in PermanentNavigationDrawerTest
Merge ND-3 (drawer contents), ND-4 (host subtitle), ND-7 (Downloads absent), and ND-8 (version absent) into a single test that opens the drawer once and checks all four assertions. Each separate test required a full AbsHarnessTestCase app launch (~3 min on loaded CI runners); merging saves three launches. Also reduce ND-2 from 3 Settings round-trips to 2 — the idempotency claim only requires showing that a second trip does not accumulate entries; a third round adds test time without adding coverage. The 8→5 reduction keeps the iOS phone harness comfortably within the 50-minute job wall observed in run 36972335588.
The base iOS harness suite takes ~40 min on main; the 50-min job wall leaves only ~10 min for new tests. Five NavDrawerTests with per-test AbsHarnessTestCase launches added ~7-9 min wall clock (3 min/launch × 5 tests on 2 parallel clones), pushing the job over the limit in both runs 36972335588 and 36977973582. Switch to class-level setUp / tearDown so each simulator clone shares a single app launch for all of its NavDrawerTests (one launch per clone instead of one per test). Wall-clock overhead drops from ~7.5 min to ~3 min — saving ~4.5 min and keeping the total below 50 min. An instance setUpWithError() recovers the app to library-home state between tests (closes any open drawer or dropdown, navigates back from Settings) so the tests remain independent despite the shared session.
…ests and AnnotationFocusHarnessTest - Rename short variable 'a' → 'app' in NavDrawerTests.swift setUp/setUpWithError to fix SwiftLint identifier_name errors (variable must be ≥3 chars) - Change 'override class func' → 'override static func' in final class to fix SwiftLint static_over_final_class warnings - Increase waitForWebViewScrollQuiet timeoutMs from 40_000 → 60_000 in AnnotationFocusHarnessTest so Phase-1 allows 30 s for Readium to start navigating on heavily loaded CI runners (previously 20 s was too short, causing false-stable pre-navigation position detection)
…in bookmark test waitForWebViewScrollQuiet was unreliable for the paginated bookmark navigation test: Phase-1 expires after 30 s if scrollLeft stays at 0 (Readium's intra-chapter JS go() fires after Phase-1), then Phase-2 returns immediately (stable at 0), leaving only 30 s for waitForPhraseOnScreen. If Readium fires at t=60+s, both windows are missed. getBoundingClientRect() flips to on-screen the moment Readium changes scrollLeft, so polling the phrase rect directly with a 90 s budget covers the same cases as the old approach but without the Phase-1/Phase-2 dead-time problem.
… setUp timeouts testDrawerContentsSubtitleAndAbsencesForAbsSource left the drawer open at the end of the test. On the slow CI clone that received the alphabetically later tests, setUpWithError()'s top-centre tap landed inside the drawer content area (not the scrim), so the drawer stayed open and the burger was never visible within the 10 s window — cascading through tests 3–5. Two fixes: - Close the drawer explicitly at the end of testDrawerContentsSubtitleAndAbsencesForAbsSource, leaving library home for subsequent tests regardless of clone assignment. - Increase both burger waitForExistence timeouts in setUpWithError() (10→15 for the first pass, 10→30 for the post-scrim-tap recovery) so the assertion survives the animation delay on a loaded CI runner.
pkmetski
force-pushed
the
pkmetski/next-multi-platform-1143
branch
from
October 2, 2026 16:27
389d12d to
a3495d1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1143
Summary
Implements full Android/iOS parity for the navigation drawer, source switcher, and shell navigation as specified in #1143.
Shared drawer composable (
feature:library-ui)NavigationDrawerComposable.ktfrom:appintofeature:library-ui(android+ios topology) so both platforms render the sameRiffleNavigationDrawercomposeResourcesplugin and drawer strings tofeature:library-uiwith ES and BG translationsRiffleNavigationDrawernow includes: dropdown source switcher, Riffle entry, source icons, username in headline, host·version subtitle, check-icon for active source, conditional Downloads link, Settings/Downloads icons, version footer, 280 dp fixed width, pinned header/footer + scrollable library list, i18n viastringResourceNavigationDrawerSourceSubtitleTest(9 tests) from:appunit tests tofeature:library-uicommonTest— exercises same code on iOS simulator in CIRiffleAppIconcomposable andic_riffle_logo.pngtofeature:design-systemfor cross-platform useiOS shell (
shared)DrawerViewModelwithNavigationDrawerViewModel(from:feature:navigation); deletesDrawerViewModel.ktandDrawerViewModelTest.ktNavigationDrawerViewModelin the iOS Koin moduleDrawerSheetContentinHomeScreen.ktwithRiffleNavigationDrawerfromfeature:library-uiLibraryHost:var nav→var navStack: List<LibraryNav>so Detail → Series/Collection/Section → Items properly pops rather than jumping to rootCircularProgressIndicator;NoLibrariesshows error text + Retry button (both translated viacomposeResources)WindowWidthSizeClass.EXPANDED)iOS tests (
iosApp/iosAppTests/NavDrawerTests.swift)Full 8-test suite wired into
iosAppTeststarget (previously orphaned from any target):DownloadsCapability)appVersion = nullon iOSAccessibility approach: Material3
ListItemwithclickableusesmergeDescendants=true— all child texts collapse into the parent button's merged label. Tests useNSPredicate(format: "label CONTAINS[c] 'Audiobookshelf'")acrossapp.descendants(matching: .any)rather thanstaticTexts["Audiobookshelf"](which fails because no separate text node exists in the merged tree).Code-review fixes
"Unable to connect to source"/"Retry"strings withstringResource(Res.string.*)so Spanish/Bulgarian users see translated textLaunchedEffectkey:drawerViewModel.redirectToLibrary(Flow) →drawerViewModel(stable VM reference)nextOverflowFontSize,buildSupportingLine,sourceSwitcherSubtitletointernalinfeature:library-ui(only used incommonTest);sourceDisplayNamestayspublic(consumed by:app'sRiffleScreen.kt)Removed tests (covered by replacements)
The
DrawerViewModelTest.ktis deleted — its 7 tests coveredDrawerViewModelwhich is also deleted. All behaviour is now covered byNavigationDrawerViewModelTestinfeature:navigationcommonTest(pre-existing), plus the newDrawerSheetContentTest(migrated to useRiffleNavigationDrawer).iOS call path
LibraryView (Swift) → HomeScreenKt.HomeScreen (KMP) → RiffleNavigationDrawer (feature:library-ui commonMain) → DrawerSheetContent → DrawerHeader / library list / footerThe
NavigationDrawerViewModelis injected viakoinInject<NavigationDrawerViewModel>()registered inshared/src/iosMain/kotlin/com/riffle/shared/Koin.kt.🤖 Generated with Claude Code