Replace BFF nav categories with static list and remove shop tab header#27
Replace BFF nav categories with static list and remove shop tab header#27hoangnhatdrk wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Shop navigation experience by replacing BFF-driven category navigation with a static category list, wiring category taps to open the native PLP with the correct collection handle, and removing the Shop segmented header (Categories/Brands/Services).
Changes:
- Replaced dynamic root navigation entries with a hardcoded list of 9 Shop categories and initialized
CategoryViewModelstate statically. - Updated category-tap navigation to route childless entries to the native PLP using
ProductListType.Category.Slug. - Removed Shop segmented tabs and rendered categories directly in
ShopScreen.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| feature/shop/src/test/java/com/mindera/alfie/feature/shop/delegate/NavigateToEntryDelegateTest.kt | Updates expectations to verify PLP navigation for childless category entries. |
| feature/shop/src/test/java/com/mindera/alfie/feature/shop/category/CategoryViewModelTest.kt | Removes dependency setup and keeps only click-handling behavior tests. |
| feature/shop/src/main/java/com/mindera/alfie/feature/shop/ShopScreen.kt | Removes segmented tab UI and renders categories directly. |
| feature/shop/src/main/java/com/mindera/alfie/feature/shop/delegate/NavigateToEntryDelegate.kt | Routes childless category entries to native PLP; preserves /brands deeplink handling. |
| feature/shop/src/main/java/com/mindera/alfie/feature/shop/category/CategoryViewModel.kt | Introduces static categories list and static state initialization. |
| feature/plp/src/test/java/com/mindera/alfie/feature/plp/ProductListViewModelTest.kt | Updates query source expectations to use the category slug as handle. |
| feature/plp/src/test/java/com/mindera/alfie/feature/plp/ProductListPreviewCountTest.kt | Updates preview count query expectations to use the category slug as handle. |
| feature/plp/src/main/java/com/mindera/alfie/feature/plp/ProductListViewModel.kt | Removes the hardcoded frontpage fallback and calls toQuerySource() directly. |
| feature/plp/src/main/java/com/mindera/alfie/feature/plp/ProductListTypeMapper.kt | Removes fallback handle parameter and maps each type to its own slug/id. |
|
Reviewed the comments from Copilot, all are not really a problem for mocked data now, and we'll automatically get rid of them once we implement the BFF-supported solution. No actions needed here. |
khoinguyen-mindera
left a comment
There was a problem hiding this comment.
Code review — Verdict: CHANGES REQUESTED
The static-list swap is clean and dropping the collectionHandle fallback is a nice simplification. Two things should be resolved before merge, flagged inline: (F1) static categories still route through a Room getByParentId lookup keyed by synthetic ids 0-8 — dead weight on fresh installs and a misroute risk on upgrades; and (F2) the /brands and /services deeplinks now silently land on the Categories list. Plus one localization nit (F3), two hygiene/coverage nits (F4, F5), and one out-of-diff UX note (F6).
🔴 F2 — feature/shop/.../ShopDeeplinks.kt (not in this diff, so noting here)
These deeplinks no longer reach their target. /brand, /brands, /services, /services/store-services, and /store-services still build shopNavArgs(ShopTab.Brands) / shopNavArgs(ShopTab.Services), but ShopScreen dropped the tab argument and no longer switches tabs — so they all now land on the Categories list. With the segmented pager gone, the Brands and Services screens are unreachable from anywhere in the app.
Depending on intent:
- If dropping Brands/Services is intentional → remove these interpreters (and the now-orphaned
ShopBrandsScreen,servicesUrl,ShopTab) here or in a fast-follow. - If not → the deeplinks need to route to whatever replaces those tabs.
Either way, leaving them pointing at an ignored tab arg is a silent regression. Can you confirm the intended behavior?
🟡 F6 — ProductListTypeMapper.kt displayTitle (out-of-diff cause)
displayTitle returns the raw slug, so the PLP top bar reads "womens-tops" while the Shop list shows "Tops". Pre-existing behavior, but this PR makes slugs the permanent titles for these categories, so the mismatch is now visible on every category. Consider mapping to a friendlier title (or threading CategoryEntryUI.title through the nav args). Low priority / follow-up.
⚠️ Verification
I couldn't run ./gradlew test / detekt in my environment (no JDK/Android SDK) — this is a static review; relying on CI for the build gate. The test edits look internally consistent ("frontpage" → "women" updated across the PLP tests; buildViewModel default is Category.Slug("women")).
| return@launch | ||
| } | ||
|
|
||
| val items = getNavEntriesByParentIdUseCase(parentId = entry.id) |
There was a problem hiding this comment.
🔴 F1 — static categories shouldn't consult the nav DB.
Each STATIC_ENTRIES item carries a synthetic id (0-8), but this still calls getNavEntriesByParentIdUseCase(entry.id) → NavigationRepository.getByParentId → the Room navigation_entries table.
After this PR nothing populates that table anymore — GetRootNavEntriesUseCase (the only writer path, via getNavEntriesByHandle) has zero callers now that it's removed from CategoryViewModel. So:
- Fresh install: the query always returns empty → a pointless suspend/DB round-trip on every tap, and the
else→Screen.Categorybranch below is unreachable dead code. - Upgrade: Room data survives app updates. A previously-shipped build populated the table with autoincrement ids starting at 1, so
getByParentId(1..8)("Men" … "Jeans") can return stale children from the old version and route the tap to the old sub-category screen instead of the PLP. ("Women", id0, is safe — Room never assigns 0.)
Since this is now a flat list of leaf categories, navigate straight to the PLP and drop the lookup + the else branch:
override fun ViewModel.openCategoryEntry(entry: CategoryEntryUI) {
viewModelScope.launch {
if (entry.path == BRANDS_FIXED_PATH) {
val environment = environmentManager.current()
deeplinkHandler.handle("${environment.webUrl}${entry.path}")
return@launch
}
runUIEvent {
navigateTo(
Screen.ProductList(
args = ProductListNavArgs(type = ProductListType.Category.Slug(entry.path))
)
)
}
}
}That also lets getNavEntriesByParentIdUseCase be dropped from the delegate's constructor. If the sub-category screen is meant to stay reachable some other way, let's discuss — right now it isn't.
There was a problem hiding this comment.
Good catch on the upgrade path — fixed. openCategoryEntry now navigates straight to the PLP for any non-/brands entry; dropped the getNavEntriesByParentIdUseCase lookup, the dead else → Screen.Category branch, and the use case from the delegate's constructor. Updated the tests accordingly (removed the obsolete "has children → Category" case). Pushed.
Side note: Screen.Category/CategoryScreen looks fully unreachable now — leaving it in scope of a follow-up rather than ripping out a whole screen here.
| private const val SCREEN_NAME = "shop_category" | ||
| private const val EVENT_LOAD_ERROR = "load_error" | ||
| private val STATIC_ENTRIES = listOf( | ||
| CategoryEntryUI(id = 0, title = StringResource.fromText("Women"), path = "women"), |
There was a problem hiding this comment.
🟡 F3 — category titles should be string resources.
These 9 titles are hardcoded English via StringResource.fromText("Women"…). The old code sourced titles from the BFF (potentially server-localized); this module already uses strings.xml for its other labels (shop_segment_*). Hardcoding literals breaks that convention and blocks localization.
Suggest StringResource.fromId(R.string.shop_category_women) etc. with entries added to strings.xml. Not a hard blocker today (no values-* translations exist yet), but cheap to do now while the list is small — or a tracked follow-up.
There was a problem hiding this comment.
Agreed on the convention. Deferring to a tracked follow-up since no values-* translations exist yet and this static list is temporary — it will be replaced when we move to the BFF-supported solution, at which point titles come from the server. Keeping it on the list rather than folding it in here.
| } | ||
|
|
||
| fun retry() = loadCategories() | ||
| fun retry() = Unit |
There was a problem hiding this comment.
🟡 F4 — retry() is now a no-op.
fun retry() = Unit does nothing, and the CategoryUIState.Error branch in ShopCategoriesScreen (wired to viewModel::retry) is unreachable since the VM only ever emits Data (STATIC_STATE). Either drop the dead retry() + Error handling, or keep real error handling if the static state is expected to become fallible later. As-is it's a confusing stub.
There was a problem hiding this comment.
Intentionally keeping retry() + the Error branch for now. Once the static list is replaced with real (fallible) data, the VM will emit Error again and this wiring becomes live — pulling it out now just means re-adding it shortly. Happy to revisit if you would rather see it gone in the interim.
| openCategoryEntry(entry) | ||
| coVerify { deeplinkHandler.handle("$WEB_URL$path") } | ||
| coVerify(exactly = 0) { deeplinkHandler.handle(any()) } | ||
| coVerify { navigateTo(screen = any(Screen.ProductList::class)) } |
There was a problem hiding this comment.
🟡 F5 — assertion doesn't verify the wired slug.
This only asserts navigateTo(any(Screen.ProductList::class)) — it doesn't verify the ProductListType.Category.Slug actually carries entry.path. Since correct path→handle wiring is the whole point of this PR, assert the value:
coVerify {
navigateTo(Screen.ProductList(ProductListNavArgs(ProductListType.Category.Slug("women"))))
}Also worth a small test covering the 9 STATIC_ENTRIES (count/order/paths) so an accidental edit to the list is caught.
There was a problem hiding this comment.
Fair point. Holding off on the stronger assertion + the STATIC_ENTRIES coverage for now since the list is throwaway mock data — the tests will need rewriting once wiring is driven by real BFF data anyway. Will land the value-level assertions with that change.
Summary
GetRootNavEntriesUseCase) with a hardcoded list of 9 categories (Women, Men, Featured, Tops, Beauty, Bags, Dresses, Jackets, Jeans), each mapped to its BFF collection handle"frontpage".ProductListTypeMapper.toQuerySource()no longer takes a fallback handle — everyProductListTyperesolves its own slug/id directlyChanges
CategoryViewModelGetRootNavEntriesUseCase,CategoryUIStateFactory,AnalyticsManager; state initialised directly from a staticSTATIC_ENTRIESlistNavigateToEntryDelegateScreen.ProductList(Category.Slug(path))instead of opening a web deeplink;/brandsdeeplink preservedProductListTypeMappercollectionHandlefallback parameter;Category.Slug,Category.Id,Brand.Slug,Brand.Ideach use their own identifier as the collection handleProductListViewModelCOLLECTION_HANDLE = "frontpage"constant and stale comment blockShopScreenShopScreenContent+SegmentedPagetabs;ShopCategoriesScreenrendered directlyTest plan
womencollectionfrontpagecollectionwomens-topscollection./gradlew testpasses./gradlew detektpasses🤖 Generated with Claude Code