GT-2584: Migrate Banners to Circuit Presenter/UI pattern#4398
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
frett
commented
Apr 22, 2026
frett
left a comment
Contributor
Author
There was a problem hiding this comment.
Code Review
Summary
Migrates FavoriteToolsBanner and TutorialFeaturesBanner from ViewModel-backed composables to Circuit BannerPresenter<T> / Layout pairs, introducing a typed Banner.UiState interface so parent presenters receive banner state directly.
Checklist Findings
✅ Looks Good
- ktlint passes
UiState,UiEvent, andeventSinkcorrectly nested inside each presenter- Both layout composables have
modifier: Modifier = Modifierand delegate all interactions viastate.eventSink(...) BannerModulecorrectly uses@Binds+@InstallIn(SingletonComponent::class)moleculeFlow+ turbine used for presenter tests — correct choice for the customBannerPresenter<T>interfaceTestEventSinkused correctly in layout testscontentKey = { it?.type }added toAnimatedContentfor correct animation keying across banner transitions@Ignoreremoved from the Paparazzi snapshot test now that no ViewModel dependency blocks it- Paparazzi snapshots committed as LFS pointers (CI-generated)
FakeBannerPresenterprovides a clean, reusable test double for parent presenter testsprivate val settings/private val eventBus— constructor parameters correctly scoped
⚠️ Minor Issues
(none)
❌ Must Fix
(none)
Overall Verdict
APPROVE — Clean migration. No remaining issues.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #4398 +/- ##
===========================================
+ Coverage 51.17% 51.29% +0.12%
===========================================
Files 448 450 +2
Lines 12612 12625 +13
Branches 2090 2093 +3
===========================================
+ Hits 6454 6476 +22
+ Misses 5534 5529 -5
+ Partials 624 620 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace direct Settings flow observation with injected BannerPresenter dependencies in HomePresenter and ToolsPresenter, enabling proper Circuit-based composition and simplifying testing via FakeBannerPresenter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
Summary
FavoriteToolsBannerandTutorialFeaturesBannercomposables with CircuitBannerPresenter<T>/ Layout pairsBanner.UiStateinterface so parent presenters (HomePresenter,ToolsPresenter) receive typed banner state directly rather than dispatching on aBannerTypeenum@Binds, enabling the previously@Ignored Paparazzi snapshot test to run without a ViewModelTest plan
FavoriteToolsBannerPresenterunit tests passTutorialFeaturesBannerPresenterunit tests passFavoriteToolsBannerLayoutandTutorialFeaturesBannerLayoutUI tests passHomePresenterTestandToolsPresenterTestpass withFakeBannerPresenterHomeLayout() - Banner - Tutorialno longer ignored)🤖 Generated with Claude Code