Jetpacker#182
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces 'JetPacker', a multi-module travel planner application featuring on-device ML Kit AI and cloud-based Firebase AI logic. Key feedback includes resolving a critical compilation error in CreateTripViewModel where a suspend function is called inside _uiState.update, and removing a Context reference to comply with the ViewModel style guide. Additionally, to prevent coroutine leaks and redundant AI tasks, autoStopJob in VoiceInputManager should be launched on the local scope, and previous generation jobs in ItineraryViewModel should be cancelled before starting new ones. Finally, several composables need adjustments to align with the style guide's Modifier parameter conventions, and the prompt in ReviewScreenViewModel should explicitly format topics instead of using the default toString() representation.
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.
| _uiState.update { it.copy(isLoading = true) } | ||
| viewModelScope.launch { | ||
| val parseDate: (String) -> Long = { dateStr -> | ||
| var parsedTime = 0L | ||
| for (pattern in listOf("MMM dd, yyyy", "yyyy-MM-dd")) { | ||
| try { | ||
| val d = SimpleDateFormat(pattern, Locale.US).parse(dateStr) | ||
| if (d != null) { | ||
| parsedTime = d.time | ||
| break | ||
| } | ||
| } catch (e: Exception) { | ||
| // continue to next pattern | ||
| } | ||
| } | ||
| parsedTime | ||
| } | ||
| _uiState.update { current -> | ||
| val startTs = parseDate(current.startDate) | ||
| val endTs = parseDate(current.endDate) | ||
|
|
||
| val newTrip = | ||
| Trip( | ||
| id = current.editingTripId ?: UUID.randomUUID().toString(), | ||
| title = current.title, | ||
| location = current.location, | ||
| startDate = startTs, | ||
| endDate = endTs, | ||
| participants = current.participants, | ||
| imageUri = current.imageUri, | ||
| ) | ||
| tripDao.insertTrip(newTrip) | ||
| current.copy( | ||
| isLoading = false, | ||
| isSuccess = true, | ||
| tripId = newTrip.id, | ||
| title = "", | ||
| location = "", | ||
| startDate = "", | ||
| endDate = "", | ||
| participants = emptyList(), | ||
| imageUri = null, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling the suspend function tripDao.insertTrip(newTrip) inside the synchronous _uiState.update { ... } block will cause a compilation error because suspend functions can only be called from a coroutine or another suspend function. Please move the database insertion outside of the update block, while keeping it inside the viewModelScope.launch coroutine. Additionally, we should handle potential date parsing failures gracefully.
_uiState.update { it.copy(isLoading = true) }
viewModelScope.launch {
val parseDate: (String) -> Long? = {
var parsedTime: Long? = null
for (pattern in listOf("MMM dd, yyyy", "yyyy-MM-dd")) {
try {
val d = SimpleDateFormat(pattern, Locale.US).parse(it)
if (d != null) {
parsedTime = d.time
break
}
} catch (e: Exception) {
// continue to next pattern
}
}
parsedTime
}
val current = _uiState.value
val startTs = parseDate(current.startDate)
val endTs = parseDate(current.endDate)
if (startTs == null || endTs == null) {
_uiState.update { it.copy(isLoading = false) }
return@launch
}
val newTrip =
Trip(
id = current.editingTripId ?: UUID.randomUUID().toString(),
title = current.title,
location = current.location,
startDate = startTs,
endDate = endTs,
participants = current.participants,
imageUri = current.imageUri,
)
tripDao.insertTrip(newTrip)
_uiState.update {
it.copy(
isLoading = false,
isSuccess = true,
tripId = newTrip.id,
title = "",
location = "",
startDate = "",
endDate = "",
participants = emptyList(),
imageUri = null,
)
}
}| var lastError: Throwable? = null | ||
|
|
||
| var lastVoiceActivityTime = System.currentTimeMillis() | ||
| val autoStopJob = scope.launch { |
There was a problem hiding this comment.
Launching autoStopJob on the parent scope instead of the child coroutine scope of listenJob can lead to a coroutine leak if listenJob is cancelled before autoStopJob is explicitly cancelled. Launch it on the local CoroutineScope (using launch instead of scope.launch) so that it is automatically cleaned up when listenJob is cancelled.
| val autoStopJob = scope.launch { | |
| val autoStopJob = launch { |
| private fun generateTripSummaryAndTips(events: List<TimelineEvent>, trip: Trip) { | ||
| viewModelScope.launch { | ||
| if (!tripSummaryAndTipsProvider.isSupported()) { | ||
| _uiState.update { it.copy(isTripSummaryAndTipsSupported = false) } | ||
| return@launch | ||
| } | ||
|
|
||
| _uiState.update { it.copy(isTripSummaryAndTipsLoading = true) } |
There was a problem hiding this comment.
Since generateTripSummaryAndTips (and other generation functions) are launched in viewModelScope without cancelling any previous active generation, rapid emissions from the combined flow (e.g., when adding events or updating themes) can trigger multiple concurrent AI generation tasks. This can lead to redundant work, race conditions, and wasted resources. Please keep track of an active Job (e.g., private var generationJob: Job? = null) and cancel the previous job before launching a new generation.
private var generationJob: Job? = null
private fun generateTripSummaryAndTips(events: List<TimelineEvent>, trip: Trip) {
generationJob?.cancel()
generationJob = viewModelScope.launch {
if (!tripSummaryAndTipsProvider.isSupported()) {
_uiState.update { it.copy(isTripSummaryAndTipsSupported = false) }
return@launch
}
_uiState.update { it.copy(isTripSummaryAndTipsLoading = true) }| open class CreateTripViewModel | ||
| @Inject | ||
| constructor( | ||
| savedStateHandle: SavedStateHandle, | ||
| private val tripDao: TripDao, | ||
| @param:ApplicationContext private val context: Context? = null, | ||
| ) : ViewModel() { |
There was a problem hiding this comment.
According to the repository style guide, ViewModels should be agnostic of the Android lifecycle and should not have references to Context to avoid memory leaks. Since context is only used in the unused private function saveBitmapToCache, both the context constructor parameter and the saveBitmapToCache function should be removed.
open class CreateTripViewModel
@Inject
constructor(
savedStateHandle: SavedStateHandle,
private val tripDao: TripDao,
) : ViewModel() {References
- Strongly recommended: ViewModels should be agnostic of the Android lifecycle. Avoid references to lifecycle-related types, Activities, Fragments, Context, or Resources. (link)
| @Composable | ||
| fun TimelineItem( | ||
| modifier: Modifier = Modifier, | ||
| event: TimelineEvent, | ||
| isLastInDay: Boolean, | ||
| isSwipedOff: Boolean, | ||
| onClick: () -> Unit, | ||
| onDelete: () -> Unit, |
There was a problem hiding this comment.
According to the repository style guide, every non-top-level composable function should take a Modifier as a parameter with a default value, positioned as the first optional parameter (after required parameters). Please move the modifier parameter to the end of the parameter list.
@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun TimelineItem(
event: TimelineEvent,
isLastInDay: Boolean,
isSwipedOff: Boolean,
onClick: () -> Unit,
onDelete: () -> Unit,
modifier: Modifier = Modifier,
) {References
- Strongly recommended: Every composable function (except top level screen composable) should take a Modifier as a parameter with a default value. It should be positionned as the first optional parameter. (link)
| fun DayHeader( | ||
| modifier: Modifier = Modifier, | ||
| date: String, | ||
| dayNumber: Int, | ||
| totalDays: Int, | ||
| isExpanded: Boolean, | ||
| onToggle: () -> Unit, | ||
| ) { |
There was a problem hiding this comment.
According to the repository style guide, every non-top-level composable function should take a Modifier as a parameter with a default value, positioned as the first optional parameter (after required parameters). Please move the modifier parameter to the end of the parameter list.
| fun DayHeader( | |
| modifier: Modifier = Modifier, | |
| date: String, | |
| dayNumber: Int, | |
| totalDays: Int, | |
| isExpanded: Boolean, | |
| onToggle: () -> Unit, | |
| ) { | |
| @Composable | |
| fun DayHeader( | |
| date: String, | |
| dayNumber: Int, | |
| totalDays: Int, | |
| isExpanded: Boolean, | |
| onToggle: () -> Unit, | |
| modifier: Modifier = Modifier, | |
| ) { |
References
- Strongly recommended: Every composable function (except top level screen composable) should take a Modifier as a parameter with a default value. It should be positionned as the first optional parameter. (link)
|
|
||
| @OptIn(ExperimentalMaterial3Api::class, ExperimentalMaterial3ExpressiveApi::class) | ||
| @Composable | ||
| fun ExpenseItem(expense: Expense, isSwipedOff: Boolean, onClick: () -> Unit, onDelete: () -> Unit) { |
There was a problem hiding this comment.
According to the repository style guide, every non-top-level composable function should take a Modifier as a parameter with a default value, positioned as the first optional parameter. Please add modifier: Modifier = Modifier to the parameter list and apply it to the root composable of ExpenseItem.
@OptIn(ExperimentalMaterial3Api::class, ExperimentalMaterial3ExpressiveApi::class)
@Composable
fun ExpenseItem(
expense: Expense,
isSwipedOff: Boolean,
onClick: () -> Unit,
onDelete: () -> Unit,
modifier: Modifier = Modifier,
) {References
- Strongly recommended: Every composable function (except top level screen composable) should take a Modifier as a parameter with a default value. It should be positionned as the first optional parameter. (link)
| val prompt = | ||
| "Generate a very short review for ${placeName}, based on these topics: ${_selectedTopics.value.joinToString(", ")}. Don't generate a title, just the body of the review. Don't use markdown. Don't extrapolate on the topics, just say if it's good or bad." |
There was a problem hiding this comment.
Using _selectedTopics.value.joinToString(", ") directly in the prompt will output the default toString() representation of the Topic data class (e.g., Topic(name=Service, positiveOpinion=true)). While the LLM might infer the meaning, it is much more reliable and cleaner to format the topics explicitly for the prompt (e.g., Service (positive)).
| val prompt = | |
| "Generate a very short review for ${placeName}, based on these topics: ${_selectedTopics.value.joinToString(", ")}. Don't generate a title, just the body of the review. Don't use markdown. Don't extrapolate on the topics, just say if it's good or bad." | |
| val topicsString = _selectedTopics.value.joinToString(", ") { | |
| "${it.name} (${if (it.positiveOpinion) "positive" else "negative"})" | |
| } | |
| val prompt = | |
| "Generate a very short review for ${placeName}, based on these topics: $topicsString. Don't generate a title, just the body of the review. Don't use markdown. Don't extrapolate on the topics, just say if it's good or bad." |
ab91aa8 to
cc9b2fa
Compare
841df5c to
d25f9c7
Compare
cc9b2fa to
36c3fec
Compare
d25f9c7 to
c1b08d7
Compare
36c3fec to
286f921
Compare
286f921 to
25b1a29
Compare
25b1a29 to
7ec106c
Compare
No description provided.