Review pr 2497#49
Conversation
…path - Merge migrateTabOrder, ensureLibrarySortDefaults, and legacy favorite migration into a single deferred coroutine (2s delay) - Defer loadPersistedDailyMix and loadSearchHistory to after first frame (1.5s delay) - This prevents DataStore first() calls and DB queries from blocking the critical init path that feeds first-frame rendering
…storage - Load songs first (most critical tab, needed for Songs tab rendering) - Chain albums after songs, artists after albums, folders after artists using Job.join() to prevent 4 simultaneous Room queries from competing for limited I/O bandwidth on eMMC storage (Redmi) - Reduces GC pressure from concurrent heap allocations
…om queries - Replace songCountFlow (Room query) with allSongsFlow (in-memory) - Now all 3 flows in the combine are in-memory StateFlows, not Room queries - Eliminates redundant SELECT COUNT(*) query that re-emitted on every songs table write during sync
- Remove individual _isLoadingCategories toggles from albums and artists jobs (they no longer manage their own loading state) - Add a single watcher coroutine that sets _isLoadingCategories = true when songs complete, then false when all sequential jobs finish - Prevents 3 redundant recompositions of LibraryScreen through playerUiState during initial data load
- Library folders/loading combine: skip update when Triple unchanged - Sort options combine: skip update when SortOptionsSnapshot unchanged - AiUiSnapshot combine: skip update when snapshot unchanged - Prevents cascading recompositions of LibraryScreen from redundant playerUiState emissions during multiple rapid state changes
…quests - Add LYRICS to AiSystemPromptType enum - Add LYRICS case to AiSystemPromptEngine.buildPrompt with role/strategy - Update translateLyrics in AiStateHolder to use AiSystemPromptType.LYRICS - Add 'Lyrics' display label to formatPromptType in AiUsageComponents - Lyrics translation requests now appear distinctly in AI activity logs
… Parameters tab - Add GENERATION_PARAMETERS category to SettingsCategory enum with Tune icon - AI Integration tab now keeps only: provider selection, API key entry, model selection, base URL, and activity logs - New Generation Parameters tab gets: system prompt editor, temperature, top P, top K, max tokens, presence/frequency penalty, sample size, digest mode, and extended song fields - Add English string resources for the new category
…earch - Add description field to AiProvider enum entries - Create SearchableProviderSelector composable similar to SearchableModelSelector with search, filtering, and count label - Replace ThemeSelectorItem in AI_INTEGRATION settings with SearchableProviderSelector for provider selection - Search covers name, displayName, and description fields
- Update AI subtitle to reflect model selection and activity logs - Add GENERATION_PARAMETERS category strings to all locales - All new strings use English as fallback pending translator contributions
… method - Remove dead hasGeminiApiKey StateFlow in PlayerViewModel - Remove dead songCountFlow StateFlow in PlayerViewModel - Remove unused getSongCountFlow() from MusicRepository interface and impl - Remove corresponding mock from PlayerViewModelTest
…pressions - Add LYRICS case to temperature selection in AiHandler.kt - Add GENERATION_PARAMETERS case to category colors in SettingsScreen.kt (both dark and light color schemes)
# Conflicts: # app/src/main/java/com/theveloper/pixelplay/presentation/model/SettingsCategory.kt # app/src/main/java/com/theveloper/pixelplay/presentation/screens/SettingsCategoryScreen.kt # app/src/main/java/com/theveloper/pixelplay/presentation/viewmodel/AiStateHolder.kt
|
@VoidX3D Testing here. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I have updated the main lyrics handling so you can now test it and check. Just let me know it any big problems arise. Its 2:35AM over here gonna sleep . |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Replace Job.join() with CompletableDeferred for infinite flow sequencing - Fix PlatformUtils reflection calls to use proper instance methods - Remove dead code detectLyricsLanguage() - Include prompt type in cache key hash - Fix loading state coordination in LibraryStateHolder - Move ensureLibrarySortDefaults() to immediate launch - Load persisted daily mix before checkAndUpdateDailyMixIfNeeded() - Restore localized AI subtitle in all 11 locale files
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Korean/Hangul lyrics were incorrectly flagged as already in English by the AI because the prompt lacked source language context. Restore detectLyricsLanguage() to generate a typed task hint (e.g. 'Translate these Korean song lyrics...').
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| viewModelScope.launch { | ||
| kotlinx.coroutines.delay(2000) | ||
| userPreferencesRepository.migrateTabOrder() | ||
| val legacyFavoriteIds = userPreferencesRepository.favoriteSongIdsFlow.first() | ||
| if (legacyFavoriteIds.isNotEmpty()) { | ||
| val roomFavoriteIds = musicRepository.getFavoriteSongIdsOnce() |
There was a problem hiding this comment.
📝 Info: Tab order migration delayed by 2 seconds may show stale tab layout briefly
migrateTabOrder() was moved from an immediate viewModelScope.launch to one with a 2-second delay(2000) at PlayerViewModel.kt:1523. During those 2 seconds after app launch, the library tab order reflects the un-migrated state. If the user opens the Library screen within that window (which is common), they may briefly see a different tab arrangement before migration completes. The comment says this is intentional ("Deferred: legacy migrations run after init to avoid blocking first render"), so this is a conscious tradeoff. For most users (those who have already migrated), this has zero effect since migrateTabOrder() is a no-op. Only affects users upgrading from older versions.
(Refers to lines 1522-1535)
Was this helpful? React with 👍 or 👎 to provide feedback.
| val combinedSystemPrompt = promptEngine.buildPrompt(basePersona, type, context) | ||
|
|
||
| val hash = (userProvider.name + combinedSystemPrompt + prompt).sha256() | ||
| val hash = (type.name + userProvider.name + combinedSystemPrompt + prompt).sha256() |
There was a problem hiding this comment.
🚩 Cache hash key uses original provider's prompt but response may come from fallback provider
In AiHandler.generateContent, the cache hash at line 185 is computed using userProvider's system prompt (combinedSystemPrompt), but the actual API call in the provider loop (lines 212-226) builds a finalSystemPrompt using each iteration's provider persona (getBasePersona(provider)). If the user's preferred provider fails and a fallback succeeds, the cached response was generated with a different system prompt than what the cache key represents. This is a pre-existing issue — the PR only adds type.name to the hash (which is a defensive improvement). Worth noting for future cache correctness work.
Was this helpful? React with 👍 or 👎 to provide feedback.
| AiSystemPromptType.LYRICS -> """ | ||
| <role>Song lyrics translator — you translate lyrics between languages while preserving structure.</role> | ||
| <strategy> | ||
| - Preserve ALL timestamps and line structure exactly. | ||
| - Output each original line followed by its translation on the next line. | ||
| - Never add explanations, labels, or formatting beyond the requested format. | ||
| - If the source is already in the target language, respond with: ALREADY_IN_TARGET_LANGUAGE | ||
| </strategy> | ||
| """.trimIndent() |
There was a problem hiding this comment.
📝 Info: LYRICS prompt type lacks an <output_schema> block unlike all other AI prompt types
Every other AiSystemPromptType in AiSystemPromptEngine.buildPrompt includes an explicit <output_schema> section (e.g., PLAYLIST returns JSON array, METADATA returns JSON object, TAGGING returns comma-separated list). The new LYRICS type at AiSystemPromptEngine.kt:186-194 only has <role> and <strategy> blocks with no <output_schema>. The output format is instead specified in the user-level prompt built in AiStateHolder.translateLyrics (AiStateHolder.kt:299-317). This works but breaks the convention where the system prompt is self-contained regarding output format. The UNIVERSAL_CONSTRAINTS block (which IS applied to LYRICS) says "Output ONLY the expected structure" — but the expected structure is only defined in the user prompt, not the system prompt.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Test changes from upstream PR
Summary by CodeRabbit
New Features
Bug Fixes