Conversation
Co-Authored-By: Weblate (bot) <info@weblate.org>
Currently translated at 21.3% (41 of 192 strings) Translation: GPTMobile/GPTMobile Translate-URL: https://hosted.weblate.org/projects/gptmobile/gptmobile/de/ Co-Authored-By: Weblate (bot) <info@weblate.org>
allow empty textFieldPrompt token customModel
Translations update from Hosted Weblate
Upgrade Ktor version to stable v3
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 117 out of 123 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- .idea/AndroidProjectSystem.xml: Language not supported
- .idea/copilot.data.migration.ask.xml: Language not supported
- .idea/deviceManager.xml: Language not supported
- .idea/kotlinc.xml: Language not supported
- .idea/markdown.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt (1)
438-441: Hardcoded user-facing string should use string resource.The error message "Failed to export chat" should be extracted to a string resource for i18n compliance, consistent with other strings in this file that use
stringResource().Proposed fix
Add to
strings.xml:<string name="export_chat_failed">Failed to export chat</string>Then update the code:
- Toast.makeText(context, "Failed to export chat", Toast.LENGTH_SHORT).show() + Toast.makeText(context, context.getString(R.string.export_chat_failed), Toast.LENGTH_SHORT).show()
🤖 Fix all issues with AI agents
In
@app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt:
- Around line 636-644: The migration is overwriting the original last-modified
time by setting updatedAt = chatRoom.createdAt; update the ChatRoomV2
construction in chatRoomV2Dao.addChatRoom to use the source entity's updatedAt
(e.g., chatRoom.updatedAt) instead of createdAt, and if chatRoom.updatedAt might
be null/absent provide a safe fallback (such as chatRoom.createdAt); modify the
ChatRoomV2 field mapping accordingly so the original updated timestamp is
preserved.
- Around line 265-276: The flow currently emits ApiState.Done twice — once when
chunk.choices?.firstOrNull()?.finishReason != null and again in onCompletion;
remove the conditional emission that checks
chunk.choices?.firstOrNull()?.finishReason and its emit(ApiState.Done) in the
chunk handling branch (e.g., in ChatRepositoryImpl's streaming/collect logic
where you check chunk.choices?.firstOrNull()?.delta/content and finishReason) so
that only the onCompletion block emits ApiState.Done; apply the same removal to
the other provider implementations that follow this pattern.
In
@app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt:
- Around line 234-242: The code risks IndexOutOfBounds on loadingStates[j] when
enabledPlatformsInChat.size != loadingStates.size; update the access to
loadingStates to a safe lookup (e.g., loadingStates.getOrElse(j) {
ChatViewModel.LoadingState.NotLoading } or loadingStates.getOrNull(j) ?:
ChatViewModel.LoadingState.NotLoading) before passing isLoading to
PlatformButton, keeping the existing conditional for i ==
groupedMessages.assistantMessages.size - 1 and still calling
chatViewModel.updateChatPlatformIndex(i, j) on click.
- Around line 603-608: Replace the hardcoded contentDescription "Remove" in the
Icon call in ChatScreen (the Icon with imageVector = Icons.Default.Close) with a
localized stringResource; update the Icon's contentDescription to
stringResource(R.string.remove) (or an appropriately named key like
R.string.close/remove_icon_description) and add that key to your strings.xml
with the proper translation(s).
🧹 Nitpick comments (13)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt (4)
626-668: Good security measures for path traversal prevention.The canonical path verification (lines 649-656) and filename sanitization are well-implemented security practices.
Two minor suggestions:
- Consider logging the exception at line 665 for debugging purposes.
- If
openInputStreamreturns null (line 628), the code creates an empty file. Consider checking for null before creating the target file.Optional: Handle null inputStream earlier
return try { val inputStream = context.contentResolver.openInputStream(uri) + ?: return null // Exit early if stream cannot be opened val rawFileName = getFileName(context, uri) val sanitizedFileName = sanitizeFileName(rawFileName)
181-181: Consider removing or guarding debug logging in production.
Log.d("ChatScreen", "GroupMessage: $groupedMessages")is called on every recomposition of the LazyColumn. This could impact performance and clutter logs in production builds.Optional: Guard with BuildConfig.DEBUG
-Log.d("ChatScreen", "GroupMessage: $groupedMessages") +if (BuildConfig.DEBUG) { + Log.d("ChatScreen", "GroupMessage: $groupedMessages") +}
702-705: Inconsistent image extension list across multiple implementations.This function (along with an identical duplicate in
ChatBubble.kt) usessetOf("jpg", "jpeg", "png", "gif", "bmp", "webp"), whileChatRepositoryImpl.isImageFileincludes additional extensions:"tiff"and"svg". This inconsistency means tiff/svg files will be treated as images in the repository layer but show a generic file icon in the UI.Additionally, the same function is duplicated across three locations (
ChatRepositoryImpl,ChatScreen, andChatBubble), which violates DRY principles.Recommend extracting
isImageFileto a shared utility/constants file to ensure consistency across the codebase.
460-467: File picker accepts all file types but only images are processed.The file picker uses
"*/*"MIME type, allowing any file to be selected. However, only image files (jpg, jpeg, png, gif, bmp, webp) are actually sent to the backend APIs. Non-image files are silently dropped during processing with no user feedback. Consider filtering the MIME type toimage/*at the picker level or validating selected files and informing users when non-image types are selected.app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/SettingRepositoryImpl.kt (1)
60-102: Migration logic is sound; consider edge case for URL normalization.The migration correctly maps V1 Platform entries to PlatformV2 with appropriate field transformations. However, the URL suffix check only handles
"v1/"(with trailing slash). If a user configured a URL like"https://api.openai.com/v1"(without trailing slash), it won't be normalized.🔧 Optional: Handle both URL suffix variants
apiUrl = if ( (platform.name == ApiType.OPENAI || platform.name == ApiType.GROQ) && - platform.apiUrl.endsWith("v1/") + (platform.apiUrl.endsWith("v1/") || platform.apiUrl.endsWith("v1")) ) { - platform.apiUrl.removeSuffix("v1/") + platform.apiUrl.removeSuffix("v1/").removeSuffix("v1") } else { platform.apiUrl },app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/GoogleAPIImpl.kt (1)
44-46: Empty API key sent when token is null.When
tokenis null, an empty string is sent as thekeyparameter. This will likely result in an API error rather than failing fast. Consider emitting an error early or at least logging a warning.🔧 Suggested fix
networkClient().preparePost(endpoint) { + if (token.isNullOrBlank()) { + throw IllegalStateException("API key is required for Google API") + } - parameter("key", token ?: "") + parameter("key", token) parameter("alt", "sse")app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.kt (2)
3-3: Unused import.
android.util.Logis imported but not used in this file.🔧 Suggested fix
package dev.chungjungsoo.gptmobile.data.network -import android.util.Log import dev.chungjungsoo.gptmobile.data.ModelConstants
47-52: Request proceeds without authentication when token is null.When
tokenis null, the request is sent without a Bearer token. This will result in a 401 error from OpenAI rather than a clear local error. This pattern is consistent across API implementations but worth considering a fail-fast approach.app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/AnthropicAPIImpl.kt (1)
32-37: Inconsistent JSON configuration usage.This implementation uses a local
jsoninstance whileOpenAIAPIImplusesNetworkClient.openAIJsonandGoogleAPIImplusesNetworkClient.json. Consider using a shared JSON configuration fromNetworkClientfor consistency and maintainability.app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (4)
71-105: Unused helper methods.
isImageFile,isDocumentFile, andgetMimeTypeare defined but never used. The transform methods useFileUtils.getMimeTypeandFileUtils.isImageinstead. Consider removing these dead code helpers.🔧 Suggested fix
- private fun isImageFile(extension: String): Boolean = extension in setOf("jpg", "jpeg", "png", "gif", "bmp", "webp", "tiff", "svg") - - private fun isDocumentFile(extension: String): Boolean = extension in setOf("pdf", "txt", "doc", "docx", "xls", "xlsx") - - private fun getMimeType(extension: String): String = when (extension) { - // Images - "jpg", "jpeg" -> "image/jpeg" - - "png" -> "image/png" - - "gif" -> "image/gif" - - "bmp" -> "image/bmp" - - "webp" -> "image/webp" - - "tiff" -> "image/tiff" - - "svg" -> "image/svg+xml" - - // Documents - "pdf" -> "application/pdf" - - "txt" -> "text/plain" - - "doc" -> "application/msword" - - "docx" -> "application/vnd.openxmlformats-officedocument.wordprocessingml.document" - - "xls" -> "application/vnd.ms-excel" - - "xlsx" -> "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" - - else -> "application/octet-stream" - }
672-680: Consider using Set for O(1) lookups in message comparison.The current implementation uses
firstOrNullfor each message, resulting in O(n²) complexity. For typical chat lengths this is acceptable, but for very long conversations, converting to Sets could improve performance.
378-394: Hardcoded token limits for reasoning mode.
maxTokens(16000) andbudgetTokens(10000) are hardcoded. Consider making these configurable viaPlatformV2or constants if different models have varying limits.
579-598: Search implementation fetches chat rooms twice.
searchChatRoomsByTitlereturns chat rooms, and thengetChatRooms()fetches all again to filter by message matches. Consider optimizing to reduce database calls.🔧 Suggested optimization
override suspend fun searchChatsV2(query: String): List<ChatRoomV2> { if (query.isBlank()) { return chatRoomV2Dao.getChatRooms() } - // Search by title - val titleMatches = chatRoomV2Dao.searchChatRoomsByTitle(query) - // Search by message content and get chat IDs val messageMatchChatIds = messageV2Dao.searchMessagesByContent(query) + + // Get all chat rooms and search by title + val allChatRooms = chatRoomV2Dao.getChatRooms() + val titleMatches = allChatRooms.filter { + it.title.contains(query, ignoreCase = true) + } - // Get all chat rooms and filter by message match IDs - val allChatRooms = chatRoomV2Dao.getChatRooms() + // Filter by message match IDs val messageMatches = allChatRooms.filter { it.id in messageMatchChatIds } // Combine results and remove duplicates, maintaining order by updatedAt return (titleMatches + messageMatches) .distinctBy { it.id } .sortedByDescending { it.updatedAt } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/proguard-rules.proapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/ModelConstants.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/AnthropicAPIImpl.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/GoogleAPIImpl.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/SettingRepositoryImpl.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/setting/AddPlatformScreen.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/setup/SetupViewModelV2.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/setting/AddPlatformScreen.kt
- app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/setup/SetupViewModelV2.kt
🧰 Additional context used
🧬 Code graph analysis (4)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/AnthropicAPIImpl.kt (2)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/GoogleAPIImpl.kt (1)
networkClient(21-115)app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.kt (1)
networkClient(28-173)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (2)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepository.kt (3)
fetchChatList(14-14)updateChatTitle(21-21)fetchMessagesV2(18-18)app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatViewModel.kt (1)
updateChatTitle(182-190)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.kt (2)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/AnthropicAPIImpl.kt (1)
networkClient(25-109)app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/GoogleAPIImpl.kt (1)
networkClient(21-115)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt (2)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatBubble.kt (4)
UserChatBubble(55-90)GPTMobileIcon(161-182)PlatformButton(184-215)isImageFile(363-366)app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (1)
isImageFile(71-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Analyze (kotlin)
- GitHub Check: Build debug APK
🔇 Additional comments (14)
app/proguard-rules.pro (2)
14-16: LGTM!Standard and appropriate
dontwarnrules for optional Netty logging dependencies. The comment clearly documents their purpose.
8-12: Remove ProGuard rules for non-existent reactor and micrometer dependencies.Ktor 3.3.3 (the version in use) does not depend on
reactororio.micrometer. These libraries are neither declared inbuild.gradle.ktsnor imported anywhere in the source code. The keep and dontwarn rules for these packages serve no purpose and should be deleted.Remove lines 8-12
# Add project specific ProGuard rules here. # You can control the set of applied configuration files using the # proguardFiles setting in build.gradle. # # For more details, see # http://developer.android.com/guide/developing/tools/proguard.html -# Keep reactive streams dependencies --keep class reactor.** { *; } --keep class io.micrometer.** { *; } --dontwarn io.micrometer.** --dontwarn reactor.** # Ignore missing optional logging dependencies used by Netty -dontwarn org.apache.log4j.** -dontwarn org.apache.logging.log4j.**Likely an incorrect or invalid review comment.
app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt (2)
683-700: Solid filename sanitization implementation.The function properly handles path traversal attempts, limits length, and provides a fallback for empty names. One edge case to consider: the function allows leading dots (e.g.,
.hiddenfile) which creates hidden files on Unix-like systems. If this is undesirable, add.trimStart('.')as well.
126-132: Verify scroll behavior fixes Issue #179.The scroll-to-end logic uses
groupedMessages.userMessages.size * 2to target the last item. This assumes the LazyColumn has exactly 2 items per user message (user bubble + assistant bubble), which matches the currentforEachIndexedstructure.However, per Issue #179, the scroll should remain pinned during streaming unless the user scrolls away. The current
LaunchedEffect(isIdle)triggers scroll only when loading state changes, which may not fully address the "pinning during streaming" requirement.Consider whether additional logic is needed to:
- Continuously scroll during streaming (not just on state change)
- Detect user scroll-away to disable auto-scroll
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/SettingRepositoryImpl.kt (1)
122-134: LGTM!The PlatformV2 CRUD operations correctly delegate to the DAO with clean, minimal implementation.
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/GoogleAPIImpl.kt (2)
79-93: LGTM!The SSE stream processing correctly handles Google's streaming format. Unlike OpenAI, Google's API doesn't send a
[DONE]marker, so relying onchannel.isClosedForReadis the appropriate termination condition.
95-127: LGTM!Network error handling is comprehensive and provides user-friendly messages. The error data classes are well-structured with appropriate defaults.
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.kt (2)
175-186: LGTM!The error data classes properly model OpenAI's error response format with appropriate optional fields.
114-116: The Responses API endpoint path is correct. The endpointPOST /responsesmatches the official OpenAI API documentation. No action required.app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/AnthropicAPIImpl.kt (2)
47-72: LGTM!The streaming implementation correctly handles Anthropic's SSE format and error responses. The error parsing properly handles Anthropic's specific error structure.
111-121: LGTM!The error data classes correctly model Anthropic's error response format.
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (2)
107-129: LGTM!The dispatch logic correctly routes requests based on
ClientType. Using the Responses API for native OpenAI and Chat Completions for compatible services is a good design choice.
281-348: LGTM!The message transformation methods correctly convert
MessageV2to provider-specific formats. Image handling with base64 encoding is properly implemented, and the logic cleanly separates text-only messages from multimodal content.app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/ModelConstants.kt (1)
17-26: Delete this suggestion—the empty string return forOLLAMAis intentional design, not an oversight.
OLLAMA_API_URLis defined as a reference constant for UI/documentation purposes, but OLLAMA URLs are user-configurable since each user runs their own local instance (potentially on different ports/machines). ThegetDefaultAPIUrlfunction returns""for OLLAMA specifically because the actual URL is retrieved from user settings viaSettingRepositoryImpl, not from a hardcoded default. The cloud APIs (OPENAI, ANTHROPIC, GOOGLE, GROQ) have fixed endpoints, so they return constants; OLLAMA does not. Changing the return value toOLLAMA_API_URLwould break this architecture and override user configurations.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt (1)
438-441: Use string resource for localization consistency.The error message is hardcoded while the rest of the file uses
stringResource(). Given that the PR adds internationalization across multiple languages, this should be localized.Proposed fix
} catch (e: Exception) { Log.e("ChatExport", "Failed to export chat", e) - Toast.makeText(context, "Failed to export chat", Toast.LENGTH_SHORT).show() + Toast.makeText(context, context.getString(R.string.export_chat_failed), Toast.LENGTH_SHORT).show() }Add corresponding string resource to
strings.xml:<string name="export_chat_failed">Failed to export chat</string>
🤖 Fix all issues with AI agents
In
@app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt:
- Around line 414-429: The flow currently emits ApiState.Done twice: once inside
the MessageStopResponseChunk branch and again in the onCompletion handler;
remove the explicit emit(ApiState.Done) from the MessageStopResponseChunk branch
in ChatRepositoryImpl so that completion is signaled only once by the existing
onCompletion handler, leaving ErrorResponseChunk handling unchanged
(emit(ApiState.Error(...))) and keeping the catch block intact.
- Around line 539-546: There’s a duplicate ApiState.Done emission: one inside
the branch that checks response.candidates?.firstOrNull()?.finishReason and
another in the Flow.onCompletion block; remove the explicit emit(ApiState.Done)
in the finishReason branch (or alternatively, remove the onCompletion emit) so
Done is emitted only once, and keep the remaining emission in the onCompletion
block to centralize completion handling; update the code paths referencing
response.candidates?.firstOrNull()?.finishReason and the Flow.onCompletion block
accordingly.
- Around line 610-618: The platforms.forEach mapping is incomplete and fragile:
update the when block in ChatRepositoryImpl (the platforms.forEach { platform ->
... } section) to include mappings for "OpenRouter" and "Custom" (assign
apiTypeMap[ApiType.OPENROUTER] and apiTypeMap[ApiType.CUSTOM] to platform.uid),
and make name matching robust (e.g., normalize platform.name to lowercase or map
names to ApiType safely instead of raw string equality) to avoid typos; also,
when constructing enabledPlatform later (the enabledPlatform =
chatRoom.enabledPlatform ... line), filter out null/blank entries by mapping via
apiTypeMap and using mapNotNull plus isNotBlank to exclude empty strings.
In
@app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt:
- Around line 234-242: The code directly indexes loadingStates[j] which can
throw IndexOutOfBounds if enabledPlatformsInChat has more entries than
loadingStates; replace that direct access in the block that builds
PlatformButton (the val isLoading calculation and its use in PlatformButton)
with a safe lookup using loadingStates.getOrElse(j) {
ChatViewModel.LoadingState.NotLoading } and then compare to
ChatViewModel.LoadingState.Loading (e.g., val isLoading =
loadingStates.getOrElse(j) { ChatViewModel.LoadingState.NotLoading } ==
ChatViewModel.LoadingState.Loading) so the PlatformButton call uses a safe
boolean instead of risking an exception.
In @app/src/main/res/values/strings.xml:
- Around line 196-210: Update the two string resources with invalid format
specifiers: replace the %1s occurrences in the string names
enabled_platform_numbers and existing_chats with the Android positional format
%1$s so they use a valid specifier (%1$s) for runtime formatting.
🧹 Nitpick comments (6)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (3)
71-105: Dead code: These helper functions are unused.
isImageFile,isDocumentFile, andgetMimeTypeare defined but never called. The code consistently usesFileUtils.getMimeType()andFileUtils.isImage()instead (e.g., lines 289-290, 444-445).🧹 Suggested removal
- private fun isImageFile(extension: String): Boolean = extension in setOf("jpg", "jpeg", "png", "gif", "bmp", "webp", "tiff", "svg") - - private fun isDocumentFile(extension: String): Boolean = extension in setOf("pdf", "txt", "doc", "docx", "xls", "xlsx") - - private fun getMimeType(extension: String): String = when (extension) { - // Images - "jpg", "jpeg" -> "image/jpeg" - - "png" -> "image/png" - - "gif" -> "image/gif" - - "bmp" -> "image/bmp" - - "webp" -> "image/webp" - - "tiff" -> "image/tiff" - - "svg" -> "image/svg+xml" - - // Documents - "pdf" -> "application/pdf" - - "txt" -> "text/plain" - - "doc" -> "application/msword" - - "docx" -> "application/vnd.openxmlformats-officedocument.wordprocessingml.document" - - "xls" -> "application/vnd.ms-excel" - - "xlsx" -> "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" - - else -> "application/octet-stream" - }
166-174: Consider extracting reasoning configuration to constants.Magic values like
effort = "medium",summary = "auto",budgetTokens = 10000(line 387), andmaxTokens = 16000(line 379) are scattered across provider implementations. Extracting these to named constants or a configuration object would improve maintainability.
577-596: Inefficient query: fetches all chat rooms twice.Line 583 fetches
titleMatchesand line 589 fetchesallChatRoomsto filter by message matches. IftitleMatchesis empty or small, you're still loading all chat rooms. Consider combining the queries or using a single query with OR conditions at the DAO level.♻️ Alternative approach
override suspend fun searchChatsV2(query: String): List<ChatRoomV2> { if (query.isBlank()) { return chatRoomV2Dao.getChatRooms() } - // Search by title - val titleMatches = chatRoomV2Dao.searchChatRoomsByTitle(query) - - // Search by message content and get chat IDs + // Get chat IDs matching by message content val messageMatchChatIds = messageV2Dao.searchMessagesByContent(query) - // Get all chat rooms and filter by message match IDs - val allChatRooms = chatRoomV2Dao.getChatRooms() - val messageMatches = allChatRooms.filter { it.id in messageMatchChatIds } - - // Combine results and remove duplicates, maintaining order by updatedAt - return (titleMatches + messageMatches) - .distinctBy { it.id } - .sortedByDescending { it.updatedAt } + // Single query: title match OR id in message matches + return chatRoomV2Dao.searchChatRoomsByTitleOrIds(query, messageMatchChatIds) }This would require adding a new DAO method that combines both conditions in a single query.
app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt (3)
592-609: Consider improving remove button touch target.The remove button has a 16dp size which is below the recommended 48dp minimum touch target for accessibility. While the visual can remain small, consider adding padding or using
Modifier.minimumInteractiveComponentSize()to improve tap accuracy.Suggested improvement
Box( modifier = Modifier .align(Alignment.TopEnd) - .size(16.dp) + .size(24.dp) .background( MaterialTheme.colorScheme.error, - RoundedCornerShape(8.dp) + RoundedCornerShape(12.dp) ) .clickable { onRemove() }, contentAlignment = Alignment.Center ) { Icon( imageVector = Icons.Default.Close, contentDescription = stringResource(R.string.remove), tint = MaterialTheme.colorScheme.onError, - modifier = Modifier.size(10.dp) + modifier = Modifier.size(14.dp) ) }
665-667: Add logging for debugging file copy failures.Silently returning
nullon exceptions makes it difficult to diagnose file attachment issues in production. Consider logging the error while still returningnullto fail gracefully.Proposed fix
targetFile.absolutePath } catch (e: Exception) { + Log.e("ChatScreen", "Failed to copy file to app directory", e) null } }
702-705: Inconsistent image extension set withChatRepositoryImpl.kt.This local implementation is missing
"tiff"and"svg"which are present inChatRepositoryImpl.isImageFile. This inconsistency means TIFF/SVG files would be treated as images in the repository but displayed with a generic file icon in the UI.Consider extracting the extension set to a shared constant or utility function.
Proposed fix
private fun isImageFile(extension: String?): Boolean { - val imageExtensions = setOf("jpg", "jpeg", "png", "gif", "bmp", "webp") + val imageExtensions = setOf("jpg", "jpeg", "png", "gif", "bmp", "webp", "tiff", "svg") return extension?.lowercase() in imageExtensions }Alternatively, create a shared constant in a common utility file to ensure consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignoreapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.ktapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build debug APK
- GitHub Check: Analyze (kotlin)
🔇 Additional comments (11)
app/src/main/res/values/strings.xml (4)
70-70: LGTM!Simple UI string for additional options menu.
99-99: LGTM!Appropriate placeholder string.
211-243: LGTM!Well-structured strings for platform configuration, file attachment, and extended thinking features. The descriptions are clear and informative for users.
244-269: LGTM!Setup wizard strings are well-organized with proper positional format specifiers on line 255 (
%1$d of %2$d). The XML comment on line 244 helps with maintainability by grouping related strings.app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (2)
107-129: Good routing pattern for multi-provider support.The
completeChatfunction cleanly routes to provider-specific implementations based onClientType. The separation between OpenAI Responses API (for native OpenAI with reasoning support) and Chat Completions API (for compatible services) is well-designed.
670-678: Message diff logic is correct for data class equality.The update detection at line 674 uses object inequality (
it != m) to identify changed messages. SinceMessageV2is a Kotlin data class (line 21 of MessageV2.kt), the generatedequals()implementation compares all properties by value, making this approach correct for detecting content changes while preserving the same message ID.app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatScreen.kt (5)
346-374: LGTM!The dropdown menu implementation is clean, and the parameter naming is now consistent with camelCase convention.
376-417: LGTM!The new
ChatBubbleDropdownMenufollows the established pattern and properly handles dismiss actions after item clicks.
460-467: LGTM!The file picker integration is well-implemented with proper null-safety checks on the URI and file path.
683-700: LGTM!The filename sanitization provides good defense-in-depth alongside the canonical path validation in
copyFileToAppDirectory. The fallback to a timestamped name ensures the function never returns an empty or problematic filename.
287-299: LGTM!The text selection bottom sheet is a nice UX addition, allowing users to select specific portions of assistant responses. The
SelectionContainerwith vertical scroll handles long content well.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/ModelConstants.kt:
- Around line 17-18: ApiType and getDefaultAPIUrl are out of sync with the
defined constants: add OPENROUTER to the ApiType enum and update
getDefaultAPIUrl to return OPENROUTER_API_URL for ApiType.OPENROUTER and return
OLLAMA_API_URL for ApiType.OLLAMA (instead of the empty string); alternatively,
if ApiType is intentionally narrower than ClientType, document that separation
and map ClientType.OPENROUTER/OLLAMA to the correct ApiType or to their
constants where AddPlatformScreen expects them. Ensure all references to
ApiType, getDefaultAPIUrl, OPENROUTER_API_URL, and OLLAMA_API_URL are updated
consistently.
In
@app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt:
- Around line 603-611: The migration is fragile because it matches
human-editable PlatformV2.name strings; instead add a stable ApiType field to
PlatformV2 (e.g., val apiType: ApiType), persist the enum value in the DB, and
update all code that creates/reads platforms (including updatePlatformName()) to
keep name editable but preserve platform.apiType; then change the mapping in
ChatRepositoryImpl (the platforms.forEach block that populates apiTypeMap) to
use platform.apiType to set apiTypeMap[ApiType.X] = platform.uid; if a schema
migration is infeasible, implement a strict validated mapping fallback:
canonicalize platform.name and log/warn on unknown names before skipping.
🧹 Nitpick comments (6)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/ModelConstants.kt (1)
7-11: Consider updating preset model lists to reflect current offerings.The preset model lists (e.g.,
openaiModels,anthropicModels) appear static. Given that the linked issue #228 mentions "None of the preset models are functional," and model identifiers evolve frequently (e.g., Claude's naming conventions changed), these lists may contain outdated model IDs.This is likely out of scope for this specific change, but worth flagging for verification against current provider documentation.
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.kt (1)
3-3: Remove unused import.The
android.util.Logimport is not used in this file.🧹 Suggested fix
-import android.util.Logapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (4)
70-104: Dead code: Unused private utility methods.The methods
isImageFile,isDocumentFile, andgetMimeTypeare defined but never called. The code usesFileUtils.getMimeTypeandFileUtils.isImageinstead (e.g., lines 288-289, 312-313, 439-440, 554-555).🧹 Remove unused methods
- private fun isImageFile(extension: String): Boolean = extension in setOf("jpg", "jpeg", "png", "gif", "bmp", "webp", "tiff", "svg") - - private fun isDocumentFile(extension: String): Boolean = extension in setOf("pdf", "txt", "doc", "docx", "xls", "xlsx") - - private fun getMimeType(extension: String): String = when (extension) { - // Images - "jpg", "jpeg" -> "image/jpeg" - - "png" -> "image/png" - - "gif" -> "image/gif" - - "bmp" -> "image/bmp" - - "webp" -> "image/webp" - - "tiff" -> "image/tiff" - - "svg" -> "image/svg+xml" - - // Documents - "pdf" -> "application/pdf" - - "txt" -> "text/plain" - - "doc" -> "application/msword" - - "docx" -> "application/vnd.openxmlformats-officedocument.wordprocessingml.document" - - "xls" -> "application/vnd.ms-excel" - - "xlsx" -> "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" - - else -> "application/octet-stream" - }
165-172: Consider making reasoning configuration values configurable.The
effort: "medium"andsummary: "auto"values are hardcoded. If users need different reasoning intensities (e.g., "low", "high"), these could be exposed as platform settings.
374-391: Hardcoded token limits for Anthropic thinking mode.Similar to the OpenAI reasoning config, the
maxTokens: 16000andbudgetTokens: 10000values are hardcoded. Consider exposing these as configurable platform settings for advanced users.
278-345: Consider extracting common file handling logic.The three transform methods (
transformMessageV2ToChatMessage,transformMessageV2ToAnthropic,transformMessageV2ToGoogle) share similar patterns for iterating files and encoding images. A private helper could reduce duplication:private fun processImageFiles(files: List<String>): List<Pair<String, String>> // mimeType, base64This is a minor refactor opportunity and can be deferred.
Also applies to: 429-465, 544-564
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/ModelConstants.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.ktapp/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.kt (2)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/GoogleAPIImpl.kt (1)
networkClient(21-115)app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/AnthropicAPIImpl.kt (1)
networkClient(25-109)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (2)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepository.kt (3)
fetchChatList(14-14)updateChatTitle(21-21)fetchMessagesV2(18-18)app/src/main/kotlin/dev/chungjungsoo/gptmobile/presentation/ui/chat/ChatViewModel.kt (1)
updateChatTitle(182-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (kotlin)
- GitHub Check: Build debug APK
🔇 Additional comments (10)
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/network/OpenAIAPIImpl.kt (4)
35-41: Consistent with other API implementations.The setters follow the same pattern as
AnthropicAPIImplandGoogleAPIImpl.
43-112: LGTM! SSE streaming implementation is correct.The implementation correctly handles:
- Endpoint URL construction with trailing slash handling
- SSE stream parsing with
[DONE]termination- HTTP error responses with structured error parsing
- Network exceptions with user-friendly error messages
The pattern is consistent with
AnthropicAPIImplandGoogleAPIImpl.
114-172: Good handling of unknown events.Unlike
streamChatCompletionwhich silently skips malformed chunks (line 88-90), this method emitsUnknownEventfor unparsable data (line 151). This is better for debugging and could be considered for the other method as well.
175-186: LGTM!The private error response classes correctly model the OpenAI API error format with appropriate optional fields.
app/src/main/kotlin/dev/chungjungsoo/gptmobile/data/repository/ChatRepositoryImpl.kt (6)
106-128: LGTM! Clean routing based on client type.The routing logic correctly directs each provider to its appropriate API implementation, with clear comments explaining the reasoning support for OpenAI's Responses API.
211-276: LGTM!The Chat Completions implementation is clean and correctly handles the OpenAI-compatible API format for Groq, Ollama, OpenRouter, and custom endpoints.
467-542: LGTM!The Google API implementation correctly handles the Gemini thinking output format by checking the
thoughtflag on each part.
570-589: LGTM! Search implementation works correctly.Minor optimization opportunity: lines 582-583 fetch all chat rooms to filter by message match IDs. If the chat list grows large, consider adding a DAO method like
getChatRoomsByIds(ids: List<Int>)to reduce memory usage.
647-679: LGTM!The save logic correctly handles both new and existing chats with proper diff calculation for messages (delete, update, add operations).
641-645: LGTM!Title handling is consistent between
generateDefaultChatTitleandupdateChatTitle- both normalize newlines and truncate to 50 characters.
Summary by CodeRabbit
New Features
Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.