feat: edit screen experiments (preview) - #195
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new 'Edit Screen' feature, enabling users to customize the layout of various screens by toggling the visibility of specific sections. This replaces the previous collapsing mechanism with a more comprehensive Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the legacy section collapsing functionality with a new Edit Screen feature, allowing users to toggle the visibility of various screen sections. The state is managed by a new EditScreenManager backed by a DataStore. Key feedback includes resolving a memory leak in EditScreenSheet caused by generating random ViewModel keys, simplifying the EditScreenViewModel and DefaultEditScreenManager by removing the synchronous isVisible cache in favor of a fully reactive flow, clearing visibility preferences on logout to prevent state leakage, using != false checks to prevent startup UI flickering, and wrapping the header more icon in an IconButton to meet accessibility touch target guidelines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| val viewModelKey = remember(active, enabledValues) { | ||
| nextInt().toString() | ||
| } | ||
|
|
||
| if (active) { | ||
| TraktBottomSheet( | ||
| sheetState = state, | ||
| onDismiss = { | ||
| onDismiss() | ||
| }, | ||
| ) { | ||
| EditScreenView( | ||
| viewModel = koinViewModel( | ||
| key = viewModelKey, | ||
| parameters = { parametersOf(enabledValues) }, | ||
| ), |
There was a problem hiding this comment.
Generating a random key via nextInt().toString() on every activation of the sheet causes a new EditScreenViewModel instance to be created and stored in the ViewModelStore every time the sheet is opened. These instances are never cleared and will accumulate in memory, leading to a memory leak. Since enabledValues is static for any given screen, you should retrieve the ViewModel without a key, allowing Koin to manage its lifecycle correctly scoped to the destination.
if (active) {
TraktBottomSheet(
sheetState = state,
onDismiss = {
onDismiss()
},
) {
EditScreenView(
viewModel = koinViewModel { parametersOf(enabledValues) },| val initialState = EditScreenState( | ||
| values = enabledValues | ||
| .associateWith { editScreenManager.isVisible(setOf(it)) } | ||
| .toImmutableMap(), | ||
| ) | ||
|
|
||
| private val valuesState = MutableStateFlow(initialState.values) | ||
|
|
||
| init { | ||
| editScreenManager.observe(enabledValues) | ||
| .onEach { values -> | ||
| valuesState.update { | ||
| values.toImmutableMap() | ||
| } | ||
| }.launchIn(viewModelScope) | ||
| } | ||
|
|
||
| fun toggle(key: EditScreenKey) { | ||
| val currentValues = valuesState.value ?: return | ||
| val isVisible = currentValues[key] ?: return | ||
|
|
||
| viewModelScope.launch { | ||
| if (isVisible) { | ||
| // Only hide if there's more than 1 visible, to prevent hiding all sections. | ||
| val visibleCount = currentValues.values.count { it } | ||
| if (visibleCount > 1) { | ||
| editScreenManager.hide(key) | ||
| } | ||
| } else { | ||
| editScreenManager.show(key) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| val state = combine( | ||
| valuesState, | ||
| ) { state -> | ||
| EditScreenState( | ||
| values = state[0], | ||
| ) | ||
| }.stateIn( | ||
| scope = viewModelScope, | ||
| started = SharingStarted.WhileSubscribed(5_000), | ||
| initialValue = EditScreenState(), | ||
| ) |
There was a problem hiding this comment.
The ViewModel can be significantly simplified by observing the EditScreenManager flow directly and mapping it to EditScreenState. This eliminates the need for initialState, valuesState, the init block, and the non-idiomatic combine call on a single flow, while also preventing potential UI flickering caused by the synchronous isVisible check during initialization. Note that you will need to add import kotlinx.coroutines.flow.map and can remove unused imports like MutableStateFlow, combine, launchIn, onEach, update.
val state = editScreenManager.observe(enabledValues)
.map { values -> EditScreenState(values.toImmutableMap()) }
.stateIn(
scope = viewModelScope,
started = SharingStarted.WhileSubscribed(5_000),
initialValue = EditScreenState(),
)
fun toggle(key: EditScreenKey) {
val currentValues = state.value.values ?: return
val isVisible = currentValues[key] ?: return
viewModelScope.launch {
if (isVisible) {
val visibleCount = currentValues.values.count { it }
if (visibleCount > 1) {
editScreenManager.hide(key)
}
} else {
editScreenManager.show(key)
}
}
}| import tv.trakt.trakt.helpers.editscreen.data.model.EditScreenKey | ||
|
|
||
| internal interface EditScreenManager { | ||
| fun isVisible(keys: Set<EditScreenKey>): Boolean |
There was a problem hiding this comment.
Since isVisible is no longer needed by the ViewModel, we can remove it from the interface to simplify the API and avoid synchronous race conditions.
| internal class DefaultEditScreenManager( | ||
| private val dataStore: DataStore<Preferences>, | ||
| scope: CoroutineScope = CoroutineScope(Dispatchers.IO + SupervisorJob()), | ||
| ) : EditScreenManager { | ||
| private val cache = ConcurrentHashMap<String, Boolean>() | ||
| private val mutex = Mutex() | ||
|
|
||
| init { | ||
| scope.launch { | ||
| val prefs = dataStore.data.first() | ||
| EditScreenKey.entries.forEach { key -> | ||
| cache[key.preferenceKey] = prefs[booleanPreferencesKey(key.preferenceKey)] ?: true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override fun isVisible(keys: Set<EditScreenKey>): Boolean { | ||
| return keys.all { key -> | ||
| cache[key.preferenceKey] ?: true | ||
| } | ||
| } | ||
|
|
||
| override fun observe(keys: Set<EditScreenKey>): Flow<Map<EditScreenKey, Boolean>> = | ||
| dataStore.data | ||
| .map { prefs -> | ||
| keys.associateWith { key -> prefs[booleanPreferencesKey(key.preferenceKey)] ?: true } | ||
| } | ||
| .distinctUntilChanged() | ||
|
|
||
| override suspend fun hide(key: EditScreenKey) { | ||
| mutex.withLock { | ||
| cache[key.preferenceKey] = false | ||
| dataStore.edit { prefs -> | ||
| prefs[booleanPreferencesKey(key.preferenceKey)] = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override suspend fun show(key: EditScreenKey) { | ||
| mutex.withLock { | ||
| cache[key.preferenceKey] = true | ||
| dataStore.edit { prefs -> | ||
| prefs[booleanPreferencesKey(key.preferenceKey)] = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override suspend fun clear() { | ||
| mutex.withLock { | ||
| cache.clear() | ||
| dataStore.edit { it.clear() } | ||
| } | ||
| } |
There was a problem hiding this comment.
Since isVisible is no longer needed, we can completely remove the synchronous cache, mutex, and background initialization coroutine. This simplifies DefaultEditScreenManager to a stateless, fully reactive wrapper around DataStore, leveraging its built-in transactional safety and thread-safe operations.
internal class DefaultEditScreenManager(
private val dataStore: DataStore<Preferences>,
) : EditScreenManager {
override fun observe(keys: Set<EditScreenKey>): Flow<Map<EditScreenKey, Boolean>> =
dataStore.data
.map { prefs ->
keys.associateWith { key -> prefs[booleanPreferencesKey(key.preferenceKey)] ?: true }
}
.distinctUntilChanged()
override suspend fun hide(key: EditScreenKey) {
dataStore.edit { prefs ->
prefs[booleanPreferencesKey(key.preferenceKey)] = false
}
}
override suspend fun show(key: EditScreenKey) {
dataStore.edit { prefs ->
prefs[booleanPreferencesKey(key.preferenceKey)] = true
}
}
override suspend fun clear() {
dataStore.edit { it.clear() }
}| internal class LogoutUserUseCase( | ||
| private val appContext: Context, | ||
| private val sessionManager: SessionManager, | ||
| private val collapsingManager: CollapsingManager, | ||
| private val checkInManager: CheckInManager, | ||
| private val apiClients: Array<ApiClient>, | ||
| private val younifyApiClient: YounifyRemoteDataSource, |
There was a problem hiding this comment.
When a user logs out, their screen visibility preferences should be cleared to prevent state leakage if another user logs in on the same device. Inject EditScreenManager here to clear it on logout.
| internal class LogoutUserUseCase( | |
| private val appContext: Context, | |
| private val sessionManager: SessionManager, | |
| private val collapsingManager: CollapsingManager, | |
| private val checkInManager: CheckInManager, | |
| private val apiClients: Array<ApiClient>, | |
| private val younifyApiClient: YounifyRemoteDataSource, | |
| internal class LogoutUserUseCase( | |
| private val appContext: Context, | |
| private val sessionManager: SessionManager, | |
| private val editScreenManager: EditScreenManager, | |
| private val checkInManager: CheckInManager, | |
| private val apiClients: Array<ApiClient>, | |
| private val younifyApiClient: YounifyRemoteDataSource, |
| suspend fun logoutUser() { | ||
| sessionManager.clear() | ||
| collapsingManager.clear() | ||
| checkInManager.stop( | ||
| source = CheckInUpdates.Source.Default, |
There was a problem hiding this comment.
Clear the screen visibility preferences on logout.
| suspend fun logoutUser() { | |
| sessionManager.clear() | |
| collapsingManager.clear() | |
| checkInManager.stop( | |
| source = CheckInUpdates.Source.Default, | |
| suspend fun logoutUser() { | |
| sessionManager.clear() | |
| editScreenManager.clear() | |
| checkInManager.stop( |
| LogoutUserUseCase( | ||
| appContext = androidApplication(), | ||
| sessionManager = get(), | ||
| collapsingManager = get(), | ||
| checkInManager = get(), | ||
| apiClients = get(named("apiClients")), | ||
| younifyApiClient = get(), |
There was a problem hiding this comment.
Pass editScreenManager = get() to the LogoutUserUseCase constructor.
| LogoutUserUseCase( | |
| appContext = androidApplication(), | |
| sessionManager = get(), | |
| collapsingManager = get(), | |
| checkInManager = get(), | |
| apiClients = get(named("apiClients")), | |
| younifyApiClient = get(), | |
| LogoutUserUseCase( | |
| appContext = androidApplication(), | |
| sessionManager = get(), | |
| editScreenManager = get(), | |
| checkInManager = get(), | |
| apiClients = get(named("apiClients")), | |
| younifyApiClient = get(), |
| onMovieClick = onMovieClick, | ||
| onMoreClick = onMoreTrendingClick, | ||
| ) | ||
| if (state.visibility?.get(DiscoverTrending) == true) { |
There was a problem hiding this comment.
Using != false instead of == true ensures that during the initial load (when state.visibility is null before the DataStore Flow emits), the sections default to being visible. This prevents a jarring blank screen flicker on startup. Please apply this pattern to all visibility checks across all screens.
| if (state.visibility?.get(DiscoverTrending) == true) { | |
| if (state.visibility?.get(DiscoverTrending) != false) { |
| Icon( | ||
| painter = painterResource(R.drawable.ic_more_vertical), | ||
| contentDescription = null, | ||
| tint = TraktTheme.colors.textPrimary, | ||
| modifier = Modifier | ||
| .size(18.dp) | ||
| .onClick { | ||
| showMenu = true | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The touch target for the more icon is only 18.dp, which is extremely small and violates Material Design accessibility guidelines (minimum 48.dp). Wrapping the Icon in an IconButton will ensure a comfortable touch target for users.
IconButton(
onClick = { showMenu = true },
modifier = Modifier.size(48.dp)
) {
Icon(
painter = painterResource(R.drawable.ic_more_vertical),
contentDescription = null,
tint = TraktTheme.colors.textPrimary,
modifier = Modifier.size(18.dp),
)
}
1 participant
No description provided.