Skip to content

perf: critical Compose recomposition hotspot fixes#2049

Merged
theovilardo merged 1 commit into
security/xxe-allowlist-ai-keys-fkfrom
perf/compose-critical-fixes
May 19, 2026
Merged

perf: critical Compose recomposition hotspot fixes#2049
theovilardo merged 1 commit into
security/xxe-allowlist-ai-keys-fkfrom
perf/compose-critical-fixes

Conversation

@lostf1sh

Copy link
Copy Markdown
Collaborator

Targets the Critical / High items from section 4 (UI/Compose performance) of the multi-agent codebase review.

Stacked on top of PR #2048. This PR's base is `security/xxe-allowlist-ai-keys-fk`. Once #2048 merges, this PR will rebase onto master automatically. The diff visible here is just the perf work.

What changed (10 task buckets)

1. LibraryScreen.kt recomp hotspots

  • Folder tab itemsToShow / songsToShow: `.toImmutableList()` now runs INSIDE the remember block; `showPlaylistCards` added to the key set (the previous key list missed `playlistMode`).
  • `rememberPagerState` was conditional on `isCompactNavigation` — lost scroll state on toggle. Replaced by a single unconditional call with a mode-aware `initialPage` + `pageCount` lambda.
  • Gradient color lists wrapped in `remember(dm, primaryContainer, onPrimaryContainer)` with `persistentListOf` so they don't re-allocate every frame.
  • `getSelectionIndex` bound method reference hoisted into a `remember(multiSelectionState)` so the three tabs receive the same lambda identity across recompositions.

2. LibraryMediaTabs.kt

  • `getAlbumColorSchemeFlow(uri)` wrapped in `remember(artUri)` — was running synchronization + dispatcher launch on every recomp inside the items lambda.
  • Placeholder `MutableStateFlow<ColorSchemePair?>(null)` allocations replaced with a file-scope `EMPTY_ALBUM_COLOR_SCHEME_FLOW`.

3. LibraryPlaybackAwareSongItem N×N flow subscriptions

  • Each item used to spin up its own `stablePlayerState.map{}.distinctUntilChanged()` collector. With 100+ visible items that is 100+ upstream subscriptions. Lifted into the parent tabs as a single `LibraryPlaybackHints(currentSongId, isPlaying)`; items receive that one instance.

4. CastBottomSheet.kt

  • `stablePlayerState` sliced to just `isPlaying` so position ticks (~4×/s) don't recompose the sheet.
  • `availableRoutes` / `bluetoothDevices` / `activeBluetoothName` / `devices` derived lists wrapped in `remember(inputs)`. `activeDevice` deliberately left inline (captures `stringResource`, composable-only).
  • The 13 `collectAsStateWithLifecycle` calls were deliberately kept separate — Compose smart-skipping already invalidates only the slice that changed; a single combine slice is not an unambiguous win.

5. QueueBottomSheet item animations (Hallazgo 3 reappeared)

  • Six independent `animateDpAsState` / `animateColorAsState` / `animateFloatAsState` calls per queue item consolidated into a single `updateTransition` keyed on a `QueueItemAnimState(isCurrentSong, isDragging, isSwipeTargeted)`. Same pattern already applied to `EnhancedSongListItem`. `dismissIconAlpha` now derives from `revealProgress × animatedFactor` so the continuous `revealProgress` doesn't need its own animation.
  • `queue` param signature: `List` → `ImmutableList` (caller already had it as ImmutableList; the downcast was erasing stability).

6. currentSongArtists stability

  • `PlayerViewModel.currentSongArtists`: `StateFlow<List>` → `StateFlow<ImmutableList>`. `FullPlayerSlice.currentSongArtists` and `FullPlayerSlicePart1.currentSongArtists` migrated to match.
  • Downstream composables — `FullPlayerSongMetadataSection`, `SongMetadataDisplaySection`, `PlayerSongInfo`, `PlayerArtistPickerBottomSheet` — now accept `ImmutableList` so Compose strong-skipping treats the parameter as stable.
  • `SongInfoBottomSheetViewModel.resolvedArtists` also migrated for the picker call site.

7. `derivedStateOf` misuses

  • `LyricsSheet.kt` had four `remember(state) { derivedStateOf { state.field } }` wrappers on plain captured values. `derivedStateOf` provides State-dependency tracking — without a State read inside, it's an extra State allocation + snapshot read for nothing. Replaced with direct destructuring.
  • `FullPlayerContent.kt:resolvedArtistId` same pattern. Replaced with plain `remember(artists, artistId)`.

8. `LyricsSheet` DataStore reads

  • Seven separate `context.dataStore.data.map{}` subscriptions (alignment, translation, romanization, animated, blur enabled, blur strength, keep-screen-on) collapsed into a single mapped `Flow` with `distinctUntilChanged`. New file-private `LyricsSheetPrefs` data class + `Preferences.toLyricsSheetPrefs()` helper.
  • Removed a duplicate `DisposableEffect` that registered an identical second lifecycle observer for keep-screen-on (merge artifact).

9. `DailyMixSection` allocations

  • `headerSongs` / `visibleSongs` called `songs.take(n).toImmutableList()` on every recomposition. Wrapped in `remember(songs)`.

Verification

  • `./gradlew :app:testDebugUnitTest`: 302 tests passing, 0 failing. Same baseline as before the perf work — no regressions.
  • `./gradlew :app:assembleDebug`: BUILD SUCCESSFUL.

What's intentionally NOT in this PR

  • `List` / `List` / `List` parameter migrations on `PlaylistBottomSheet`, `PlaylistArtCollage`, `MultiSelectionBottomSheet`, `AlbumMultiSelectionOptionSheet`, `PlaylistContainer`. Tried and reverted — each has ~10 call sites passing plain `playlist.songs: List`. Flipping the parameter requires either `.toImmutableList()` boilerplate at every site (anti-pattern) or migrating `Playlist.songs` upstream to `ImmutableList`. The second is the proper fix and belongs to a separate task once the source data model is touched.
  • `LyricsSheet` architectural violation. The 7 reads are consolidated into one flow but still bypass `UserPreferencesRepository`. Properly routing them through the repository needs new flow exposures there and is its own task.
  • `CastBottomSheet` 13 `collectAsStateWithLifecycle` consolidation. Compose smart skipping already invalidates only the slice that changed; a single combine slice was not an unambiguous win.
  • `triggerShuffleAllFromTile` behavioral change is in DO NOT MERGE - security: close XXE, Cast LAN exposure, AI key plaintext, song FK constraint #2048, not here. It belongs there because it was the fix for a failing unit test.

Reviewer notes

  • The single largest perf win is probably the `LibraryPlaybackAwareSongItem` lift — going from N collectors to 1 across the songs/favorites tabs.
  • The `updateTransition` consolidation in `QueueBottomSheet` mirrors the EnhancedSongListItem refactor that was already in master. Worth keeping the pattern consistent.
  • `LibraryPlaybackHints` is marked `@Immutable` — `compose_stability.conf` only covers `data.model.**`, so this annotation is needed explicitly.
  • The `gradient*Colors` change uses `persistentListOf` instead of `toImmutableList()` because the input is a small literal list and `persistentListOf` avoids an extra wrap step.
  • The reverted `PlaylistArtCollage` / `PlaylistBottomSheet` migrations are still tracked as TODO in the codebase — they're called out in the commit message's "What is NOT in this PR" section so the next maintainer doesn't redo the same revert cycle.

Targets the Critical / High items from section 4 (UI/Compose
performance) of the multi-agent codebase review. Each fix below is
small and isolated; the build still produces a passing 302-test
unit suite and a clean :app:assembleDebug.

LibraryScreen.kt
- Folder tab itemsToShow / songsToShow: .toImmutableList() now runs
  INSIDE the remember block instead of being chained outside it, and
  showPlaylistCards is added to the key set (the previous key list
  missed playlistMode, so the cached list could go stale on toggle).
- rememberPagerState was being called in two branches of an if/else,
  which loses scroll state when libraryNavigationMode toggles between
  COMPACT_PILL and the full tab strip. Replaced by a single call with
  a mode-aware initialPage + pageCount lambda.
- Gradient color lists used inline listOf().toImmutableList() on every
  recomp. Wrapped in remember(dm, primaryContainer, onPrimaryContainer)
  using persistentListOf so the list isn't re-allocated per frame.
- getSelectionIndex bound method reference is now hoisted into a
  remember(multiSelectionState) so the same lambda identity is passed
  to LibrarySongsTab / LibraryFavoritesTab / LibraryFoldersTab on every
  recomposition. Previously each tab received a fresh lambda, breaking
  Compose parameter stability on those tabs.

LibraryMediaTabs.kt
- getAlbumColorSchemeFlow(uri) was called inside the items lambda for
  every visible album on every recomposition. Each call synchronizes
  pendingAlbumColorSchemeLock and dispatcher-launches a generation
  coroutine on a cache miss. Wrapped in remember(artUri) so the lookup
  is amortized per album.
- Placeholder branches were allocating a fresh
  MutableStateFlow<ColorSchemePair?>(null) on every render. Replaced
  with a single file-scope EMPTY_ALBUM_COLOR_SCHEME_FLOW.

LibraryPlaybackAwareSongItem.kt + LibrarySongsTab.kt + LibrarySongsAndFavoritesTabs.kt
- Each item used to spin up its own stablePlayerState.map{}.
  distinctUntilChanged() collector. With 100+ items visible across
  tabs + paging buffers that is 100+ upstream subscriptions each
  checking every emission. Lifted the collection into the parent
  tabs as a single LibraryPlaybackHints(currentSongId, isPlaying)
  flow; items now receive that one hints instance.

CastBottomSheet.kt
- stablePlayerState was being collected just to read isPlaying. Sliced
  with .map { it.isPlaying }.distinctUntilChanged() so position ticks
  (~4×/s) don't recompose the sheet.
- availableRoutes / bluetoothDevices / activeBluetoothName / devices
  derived lists were rebuilt inline on every recomposition. Wrapped
  each in remember(inputs). activeDevice was deliberately left inline
  because it captures stringResource (composable-only).

QueueBottomSheet.kt
- Hallazgo 3 reappeared in QueuePlaylistSongItem: six independent
  animateDpAsState / animateColorAsState / animateFloatAsState calls
  per visible queue item. Consolidated into a single updateTransition
  keyed on a QueueItemAnimState(isCurrentSong, isDragging,
  isSwipeTargeted) — same pattern that was applied to
  EnhancedSongListItem. dismissIconAlpha now derives from
  revealProgress × an animated factor, so revealProgress can be a
  plain float instead of needing its own animation.
- queue param signature: List<Song> -> ImmutableList<Song>. The
  caller in UnifiedPlayerOverlaysLayer already had it as
  ImmutableList; the downcast there was erasing stability info.

PlayerViewModel.kt + downstream composables
- currentSongArtists: StateFlow<List<Artist>> -> StateFlow<
  ImmutableList<Artist>>. FullPlayerSlice.currentSongArtists and
  FullPlayerSlicePart1.currentSongArtists migrated to match.
- FullPlayerSongMetadataSection, SongMetadataDisplaySection,
  PlayerSongInfo (all FullPlayerContent.kt) and
  PlayerArtistPickerBottomSheet now accept ImmutableList<Artist>.
- SongInfoBottomSheetViewModel.resolvedArtists also moved to
  ImmutableList<Artist> for the picker call site.

LyricsSheet.kt
- Seven separate context.dataStore.data.map{} subscriptions
  (alignment, translation, romanization, animated, blur enabled,
  blur strength, keep-screen-on) collapsed into a single mapped
  Flow<LyricsSheetPrefs> with distinctUntilChanged. New file-private
  LyricsSheetPrefs data class + Preferences.toLyricsSheetPrefs()
  helper. The architectural-violation note (these reads still bypass
  UserPreferencesRepository) is documented in a comment; the proper
  fix needs new repository flows and is a separate task.
- Removed a duplicate DisposableEffect that registered an identical
  second lifecycle observer for keep-screen-on (merge artifact).
- Four remember(state) { derivedStateOf { state.field } } wrappers
  on plain captured values (isLoadingLyrics, lyrics, isPlaying,
  currentSong) replaced with direct destructuring. derivedStateOf
  with no State<T> read inside is dead weight.

FullPlayerContent.kt
- Same derivedStateOf misuse on resolvedArtistId. Calculation reads
  only the artists parameter and captured artistId, no State<T>.
  Replaced with plain remember(artists, artistId).

DailyMixSection.kt
- DailyMixCard's headerSongs / visibleSongs were calling
  songs.take(n).toImmutableList() on every recomposition. Wrapped
  both in remember(songs).

Verification
- ./gradlew :app:testDebugUnitTest passes 302 tests, 0 failing.
- ./gradlew :app:assembleDebug succeeds.

What is NOT in this PR
- The List<Song> / List<Album> / List<Playlist> parameter migrations
  on PlaylistBottomSheet, PlaylistArtCollage, MultiSelectionBottomSheet,
  AlbumMultiSelectionOptionSheet, and PlaylistContainer were tried
  and reverted. Each has ~10 call sites across LibraryScreen,
  AlbumDetailScreen, ArtistDetailScreen, DailyMixScreen,
  GenreDetailScreen, etc. that all pass plain playlist.songs:
  List<Song>. Flipping the parameter requires either toImmutableList()
  boilerplate at every site (anti-pattern) or migrating
  Playlist.songs upstream to ImmutableList. The second migration is
  the proper fix and belongs to a separate task once the source data
  model is touched.
- The architectural violation in LyricsSheet (direct DataStore reads
  bypassing UserPreferencesRepository) is consolidated but not yet
  routed through the repository.
- The 13 collectAsStateWithLifecycle calls in CastBottomSheet were
  deliberately kept separate. Compose smart-skipping already
  invalidates only the slice that changed; consolidating them into a
  single combine slice was not an unambiguous win.
@lostf1sh

Copy link
Copy Markdown
Collaborator Author

@copilot review

Copilot AI commented May 19, 2026

Copy link
Copy Markdown
Contributor

@copilot review

Reviewed at b891e23. No additional code changes are needed from this follow-up. I also checked Actions runs: the only failure on the base branch was an infra runner shutdown in CodeQL (not a code regression), and recent runs are green/in progress. Local :app:testDebugUnitTest could not be rerun here due toolchain download/network limits (foojay HEAD failure), not due test failures.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.foojay.io
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java -Xmx64m -Xms64m -Dorg.gradle.appname=gradlew -jar /home/REDACTED/work/PixelPlayer/PixelPlayer/gradle/wrapper/gradle-wrapper.jar :app:testDebugUnitTest (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@theovilardo theovilardo merged commit b99468d into security/xxe-allowlist-ai-keys-fk May 19, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants