π :: μ μ² νμ΄μ§ ꡬν #815
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. π WalkthroughWalkthroughAdds Remain and Vote features with navigation keys, a CompositionLocal-backed ResultStore for cross-destination results, new models/mappers/repos/DI, multiple Compose UI components and screens, ViewModels, network signature updates, design-system primitives/resources, and small build/minSdk adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplicationScreen
participant ResultStore
participant NavDisplay
participant RemainScreen
participant RemainVM
participant RemainsRepository
User->>ApplicationScreen: open Application tab
ApplicationScreen->>ResultStore: read "remain_application_result"
Note right of ApplicationScreen `#e6f7ff`: show applied badge if present
User->>ApplicationScreen: tap "μλ₯" card
ApplicationScreen->>NavDisplay: push RemainScreenNav
NavDisplay->>RemainScreen: render RemainApplication
RemainScreen->>RemainVM: load options & time
RemainVM->>RemainsRepository: fetchRemainsOptions()/fetchRemainsApplicationTime()
RemainsRepository-->>RemainVM: returns data
User->>RemainScreen: select option & confirm
RemainScreen->>RemainVM: changeRemainsOption()
RemainVM->>RemainsRepository: updateRemainsOption(id)
RemainsRepository-->>RemainVM: success
RemainVM->>ResultStore: set("remain_application_result", selectedTitle)
RemainVM->>NavDisplay: navigateBack()
NavDisplay->>ApplicationScreen: resumed
ApplicationScreen->>ResultStore: read "remain_application_result"
ResultStore-->>ApplicationScreen: selectedTitle
Estimated code review effortπ― 4 (Complex) | β±οΈ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (2 passed)
β¨ Finishing touches
π§ͺ Generate unit tests (beta)
π Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro π Files selected for processing (2)
π§ Files skipped from review as they are similar to previous changes (2)
β° 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). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @uson1004, 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! μ΄ Pull Requestλ μλ‘μ΄ "μ μ²" νμ΄μ§λ₯Ό ꡬννκ³ , μ΄ νμ΄μ§ λ΄μμ μλ₯ μ μ² κΈ°λ₯κ³Ό ν¬ν κΈ°λ₯μ μ 곡ν©λλ€. μ¬μ©μ κ²½νμ κ°μ νκΈ° μν΄ μλ‘μ΄ UI μ»΄ν¬λνΈλ€μ΄ μΆκ°λμμΌλ©°, νλ©΄ κ° λ°μ΄ν° μ λ¬μ μν Highlights
π§ New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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
μ΄ PRμ μ μ² νμ΄μ§λ₯Ό ꡬννλ©°, μλ₯ μ μ² λ° ν¬ν λͺ©λ‘ κΈ°λ₯μ ν¬ν¨ν©λλ€. μλ‘μ΄ UI μ»΄ν¬λνΈ, λ΄λΉκ²μ΄μ λ‘μ§, λ·°λͺ¨λΈ, λ°μ΄ν° κ³μΈ΅μ΄ μΆκ°λμμ΅λλ€. μ λ°μ μΈ κ΅¬μ‘°λ μ’μ§λ§, μ½λ νμ§, μμ μ±, μ μ§λ³΄μμ± μΈ‘λ©΄μμ κ°μ ν λΆλΆμ΄ λͺ κ°μ§ μμ΅λλ€. ꡬ체μ μΈ νΌλλ°±μΌλ‘λ μμ νμ§ μμ API μ€κ³, λ§€μ§ μ€νΈλ§ μ¬μ©, μλͺ»λ μν κ΄λ¦¬, μΌκ΄μ± μλ λ°μ΄ν° λͺ¨λΈλ§ λ±μ λ¬Έμ λ₯Ό μ§μ νμ΅λλ€.
| viewModelScope.launch { | ||
| remainsRepository.fetchRemainsOptions().onSuccess { remainsOptions -> | ||
| val selectRemainsOptionId = | ||
| remainsOptions.find { it.applied }?.id ?: UUID.randomUUID() |
There was a problem hiding this comment.
νμ¬ μ μ©λ μ΅μ
μ΄ μλ κ²½μ° UUID.randomUUID()λ₯Ό μ¬μ©νμ¬ μμμ UUIDλ₯Ό μμ±ν©λλ€. μ΄λ μλ―Έμ μ¬λ°λ₯΄μ§ μμΌλ©° νΌλμ μΌκΈ°ν μ μμ΅λλ€. μ νλ μ΅μ
μ΄ μμμ λνλ΄κΈ° μν΄ nullμ μ¬μ©νλ κ²μ΄ λ μ’μ΅λλ€. selectRemainsOptionId μμ±μ nullable(UUID?)λ‘ λ§λ€μ΄μΌ ν©λλ€.
| remainsOptions.find { it.applied }?.id ?: UUID.randomUUID() | |
| remainsOptions.find { it.applied }?.id |
...rc/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt
Outdated
Show resolved
Hide resolved
| inline fun <reified T> getResultState(resultKey: String = T::class.toString()) = | ||
| resultStateMap[resultKey]?.value as T |
There was a problem hiding this comment.
getResultState ν¨μλ νμ
μΊμ€ν
μ΄ μμ νμ§ μμ΅λλ€. resultStateMap[resultKey]κ° nullμ΄κ±°λ νμ
μ΄ μΌμΉνμ§ μμΌλ©΄ as T μΊμ€ν
μΌλ‘ μΈν΄ μ±μ΄ μΆ©λν μ μμ΅λλ€. as? Tλ₯Ό μ¬μ©νκ³ λ°ν νμ
μ T?λ‘ λ³κ²½νμ¬ λ μμ νκ² λ§λ€ μ μμ΅λλ€.
| inline fun <reified T> getResultState(resultKey: String = T::class.toString()) = | |
| resultStateMap[resultKey]?.value as T | |
| inline fun <reified T> getResultState(resultKey: String = T::class.toString()): T? = | |
| resultStateMap[resultKey]?.value as? T |
data/src/dev/kotlin/team.aliens.dms.android.data/remain/model/RemainsApplicationTime.kt
Show resolved
Hide resolved
|
|
||
| LaunchedEffect(Unit) { | ||
| snapshotFlow { | ||
| resultStore.resultStateMap["remain_application_result"]?.value as? String? |
There was a problem hiding this comment.
ResultStoreμ ν€λ‘ μ¬μ©λλ λ¬Έμμ΄ λ¦¬ν°λ΄ "remain_application_result"κ° νλμ½λ©λμ΄ μμ΅λλ€. μ΄ ν€λ DmsApp.ktμμλ μ¬μ©λ©λλ€. μ΄λ° 'λ§€μ§ μ€νΈλ§'μ μ€νλ₯Ό μ λ°νκ³ μ μ§λ³΄μλ₯Ό μ΄λ ΅κ² λ§λλλ€. const valλ‘ μμλ₯Ό μ μνμ¬ μ¬λ¬ κ³³μμ μ¬μ¬μ©νλ κ²μ΄ μ’μ΅λλ€.
| listOf( | ||
| Triple("μλ₯", R.drawable.img_home, onNavigateRemainApplication), | ||
| Triple("μΈμΆ μ μ²νκΈ°", R.drawable.img_outing, onNavigateOutingApplication), | ||
| Triple("λ΄μ¬ νλ μ μ²νκΈ°", R.drawable.img_volunteer, onNavigateVolunteerApplication), | ||
| ).forEach { (title, icon, onClick) -> | ||
| DmsApplicationCard( | ||
| title = title, | ||
| iconRes = icon, | ||
| onClick = onClick, | ||
| appliedTitle = if (title == "μλ₯") appliedTitle else null, | ||
| ) | ||
| } |
There was a problem hiding this comment.
μ μ² νλͺ© λͺ©λ‘μ΄ listOf(Triple(...))λ‘ νλμ½λ©λμ΄ μκ³ , if (title == "μλ₯")μ κ°μ λ‘μ§μμ 'λ§€μ§ μ€νΈλ§'μ μ¬μ©νκ³ μμ΅λλ€. μ΄λ μ»΄ν¬λνΈμ μ¬μ¬μ©μ±μ λ¨μ΄λ¨λ¦¬κ³ μ μ§λ³΄μλ₯Ό μ΄λ ΅κ² λ§λλλ€. μ μ² νλͺ©μ λν λ°μ΄ν° ν΄λμ€λ₯Ό μ μνκ³ μ΄ λͺ¨λΈμ 리μ€νΈλ₯Ό μ λ¬νλ κ²μ΄ λ μ’μ΅λλ€. appliedTitleμ νμνλ λ‘μ§μ ν΄λΉ λͺ¨λΈμ Boolean νλκ·Έλ‘ μ²λ¦¬ν μ μμ΅λλ€.
.../src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.kt
Outdated
Show resolved
Hide resolved
| }.onFailure { | ||
| // Logger.a(it) { it.message.toString() } | ||
| } |
There was a problem hiding this comment.
λ€νΈμν¬ νΈμΆμ onFailure λΈλ‘μ΄ λΉμ΄ μμ΅λλ€(μ£Όμ μ²λ¦¬λ¨). μ€λ₯λ₯Ό λ‘κΉ
νκ±°λ μ¬μ©μμκ² λ©μμ§λ₯Ό νμνλ λ± μ€λ₯λ₯Ό μ μ ν μ²λ¦¬νμ¬ μ±μ μμ μ±μ λμ΄κ³ λλ²κΉ
μ μ©μ΄νκ² ν΄μΌ ν©λλ€.
| val icon = when (remainsOption.title) { | ||
| "κΈμμΌ κ·κ°" -> R.drawable.img_night_bus | ||
| "ν μμΌ κ·κ°" -> R.drawable.img_bus | ||
| "ν μμΌ κ·μ¬" -> R.drawable.img_home | ||
| "μ£Όλ§ μλ₯" -> R.drawable.img_small_home | ||
| else -> R.drawable.img_bus | ||
| } | ||
| val appliedTitle = if (remainsOption.applied) "μ μ²λ¨" else null |
There was a problem hiding this comment.
μμ΄μ½μ κ²°μ νκΈ° μν΄ when λ¬Έμμ "κΈμμΌ κ·κ°"μ κ°μ 'λ§€μ§ μ€νΈλ§'μ μ¬μ©νκ³ μμ΅λλ€. μ΄λ κ²¬κ³ νμ§ λͺ»ν λ°©μμ
λλ€. RemainsOption λ°μ΄ν° λͺ¨λΈμ μμ΄μ½ 리μμ€ IDλ μμ΄μ½μ λνλ΄λ enumμ ν¬ν¨νλ κ²μ΄ λ μ’μ΅λλ€. λν, "μ μ²λ¨" λ¬Έμμ΄μ΄ νλμ½λ©λμ΄ μμ΅λλ€. νμ§ν λ° κ΄λ¦¬λ₯Ό μν΄ λ¬Έμμ΄ λ¦¬μμ€λ‘ μΆμΆν΄μΌ ν©λλ€.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
π§Ή Nitpick comments (19)
data/src/dev/kotlin/team.aliens.dms.android.data/voting/model/StudentGcnInfo.kt (1)
3-6: Consider adding serialization annotations.If this model is used for network requests/responses or database storage, you'll need serialization annotations. For example, with kotlinx.serialization:
π Example with kotlinx.serialization
+import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable + +@Serializable data class StudentGcnInfo( + @SerialName("student_gcn") val studentGcn: String, + @SerialName("student_filter_id") val studentFilterId: Int, )Note: Adjust
@SerialNamevalues to match your actual API field names if they differ from the property names.Optionally, add KDoc for clarity.
Example documentation
+/** + * Student GCN (Grade-Class-Number) information for voting. + * @property studentGcn The student's GCN identifier + * @property studentFilterId The filter ID associated with the student + */ data class StudentGcnInfo( val studentGcn: String, val studentFilterId: Int, )core/ui/src/dev/java/team/aliens/dms/android/core/ui/util/Date.kt (1)
6-6: Consider using string resources for potential i18n support.The date format hardcodes Korean locale strings ("λ ", "μ", "μΌ"). While this is acceptable if the app is Korea-only, consider using string resources if there's any possibility of supporting multiple locales in the future.
Example using string resources
In
strings.xml:<string name="date_format">%1$dλ %2$dμ %3$dμΌ</string>In code:
fun LocalDateTime.toDateString(context: Context): String { return context.getString(R.string.date_format, this.year, this.monthValue, this.dayOfMonth) }Note: This requires passing a
Contextparameter, which may not be ideal for a utility function. Alternatively, keep the current approach if the app will remain Korean-only.core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt (1)
110-121: Consider adding observability for authentication failures.The
reissueTokens()method now silently handles 401 errors by clearing caches, which is correct behavior. However, adding logging or metrics would help track authentication issues in production (e.g., frequency of token reissue failures, 401 responses, etc.).This would improve debugging and help identify patterns in auth-related issues.
data/src/dev/kotlin/team.aliens.dms.android.data/remain/model/RemainsApplicationTime.kt (1)
5-10: Consider removing default values or using more type-safe time representation.The default values (SUNDAY and empty strings) create an instance that doesn't represent a valid time window. Consider either:
- Removing defaults to force callers to provide meaningful values
- Using a more type-safe type like
LocalTimeinstead ofStringfor time fields to enforce format validationπ Alternative using LocalTime
package team.aliens.dms.android.data.remain.model -import java.time.DayOfWeek +import java.time.DayOfWeek +import java.time.LocalTime data class RemainsApplicationTime( - val startDayOfWeek: DayOfWeek = DayOfWeek.SUNDAY, - val startTime: String = "", - val endDayOfWeek: DayOfWeek = DayOfWeek.SUNDAY, - val endTime: String = "", + val startDayOfWeek: DayOfWeek, + val startTime: LocalTime, + val endDayOfWeek: DayOfWeek, + val endTime: LocalTime, )data/src/dev/kotlin/team.aliens.dms.android.data/voting/model/ModelStudentCandidates.kt (1)
5-10: Consider nullability forprofileImageUrl.If students may not always have a profile image, consider making
profileImageUrlnullable (String?) to prevent potential empty string handling or runtime issues when the URL is missing from the backend response.core/ui/src/dev/java/team/aliens/dms/android/core/ui/navigation/ResultStore.kt (1)
56-56: Consider makingresultStateMapinternal or private.Exposing
resultStateMapas public allows external code to modify the internal state directly, bypassing the providedsetResult/removeResultmethods.π Proposed fix
- val resultStateMap: MutableMap<String, MutableState<Any?>> = mutableMapOf() + internal val resultStateMap: MutableMap<String, MutableState<Any?>> = mutableMapOf()feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.kt (1)
13-13: Unused import.
toDateStringis imported but not used in this file.π Proposed fix
-import team.aliens.dms.android.core.ui.util.toDateStringapp/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
161-170: Extract result key to a constant.The hardcoded string
"remain_application_result"should be extracted to a named constant to prevent typos and ensure consistency when reading this result elsewhere (e.g., inApplicationRoute).π Proposed fix
Add a constant at the top level or in a companion object:
private const val REMAIN_APPLICATION_RESULT_KEY = "remain_application_result"Then use it:
- resultStore.setResult<String?>("remain_application_result", title) + resultStore.setResult<String?>(REMAIN_APPLICATION_RESULT_KEY, title)feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/component/DmsFloatingNotice.kt (2)
49-52: Consider adding a tint to the Icon for visual consistency.The
Icondoesn't have atintspecified, so it will use the defaultLocalContentColor. Since the text usesDmsTheme.colorScheme.onBackground, the icon may not match visually. Consider explicitly setting the tint.π Proposed fix
Icon( painter = painterResource(iconResource), contentDescription = null, + tint = DmsTheme.colorScheme.onBackground, )
58-61: Review minimum font size for readability.A
minFontSizeof1.spmay result in unreadable text on some devices. Consider using a higher minimum (e.g.,8.spor10.sp) to ensure accessibility.π Proposed fix
autoSize = TextAutoSize.StepBased( - minFontSize = 1.sp, + minFontSize = 10.sp, maxFontSize = 50.sp, ),core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.kt (1)
44-50: Consider adding a label to the animation for debugging.Adding a
labelparameter helps identify this animation in Android Studio's Animation Preview and Compose tooling.π Proposed fix
val borderColor by animateColorAsState( targetValue = if (isSelected) { DmsTheme.colorScheme.onPrimaryContainer } else { DmsTheme.colorScheme.surfaceTint }, + label = "DmsApplicationCard_borderColor", )feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/viewmodel/ApplicationViewModel.kt (1)
21-22: Consider abstracting SharedPreferences access.Directly using
SharedPreferencesin the ViewModel reduces testability. Consider injecting aDataStoreor a custom repository/preferences wrapper that can be mocked in tests.feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (2)
3-3: Remove unused import.
android.util.Logis imported but never used in this file.π Proposed fix
-import android.util.Log
88-94: Hardcoded string matching for icon selection is fragile.Mapping icons based on exact Korean title strings (
"κΈμμΌ κ·κ°","ν μμΌ κ·κ°", etc.) is brittle. If the backend returns slightly different titles or the text changes, the icon mapping silently falls back to the default.Consider using an enum, type field, or identifier from the
remainsOptionmodel to determine the icon instead of relying on display text matching.data/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepository.kt (2)
9-23: Consider using an interface instead of abstract class.Kotlin idiom prefers interfaces for repository contracts when there's no shared implementation logic. This provides more flexibility for testing (mocking) and allows implementing multiple interfaces.
π Proposed change
-abstract class VotingRepository { - - abstract suspend fun fetchAllVoteSearch(): Result<List<AllVoteSearch>> - - abstract suspend fun fetchCheckVotingItem(votingTopicId: UUID): Result<List<VotingItem>> - - abstract suspend fun fetchCreateVotingItem( - votingTopicId: UUID, - selectedId: UUID, - ): Result<Unit> - - abstract suspend fun fetchDeleteVotingItem(voteId: UUID): Result<Unit> - - abstract suspend fun fetchModelStudentCandidates(requestDate: LocalDate): Result<List<ModelStudentCandidates>> -} +interface VotingRepository { + suspend fun fetchAllVoteSearch(): Result<List<AllVoteSearch>> + suspend fun fetchCheckVotingItem(votingTopicId: UUID): Result<List<VotingItem>> + suspend fun createVotingItem(votingTopicId: UUID, selectedId: UUID): Result<Unit> + suspend fun deleteVotingItem(voteId: UUID): Result<Unit> + suspend fun fetchModelStudentCandidates(requestDate: LocalDate): Result<List<ModelStudentCandidates>> +}
15-20: Misleading "fetch" prefix on mutation methods.
fetchCreateVotingItemandfetchDeleteVotingItemuse the "fetch" prefix which typically implies a read operation. These are write operations that should use more appropriate naming likecreateVotingItemanddeleteVotingItem.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (2)
3-3: Remove unused import.
android.util.Logis imported but never used in this file.π Proposed fix
-import android.util.Log
43-51: Magic string for result key.The string
"remain_application_result"is used directly. Consider extracting this to a constant to avoid typos and improve maintainability, especially since it's likely used elsewhere (e.g., when setting the result).feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
31-42: Refactor to avoid fragile string comparison.The conditional logic on line 40 relies on string matching (
title == "μλ₯") to determine which card should display theappliedTitle. If the title on line 32 is modified, this logic will silently break. Consider using an index-based check or a data class with an explicit flag to make the code more robust and maintainable.π Proposed refactor using index-based approach
- listOf( + listOf( Triple("μλ₯", R.drawable.img_home, onNavigateRemainApplication), Triple("μΈμΆ μ μ²νκΈ°", R.drawable.img_outing, onNavigateOutingApplication), Triple("λ΄μ¬ νλ μ μ²νκΈ°", R.drawable.img_volunteer, onNavigateVolunteerApplication), - ).forEach { (title, icon, onClick) -> + ).forEachIndexed { index, (title, icon, onClick) -> DmsApplicationCard( title = title, iconRes = icon, onClick = onClick, - appliedTitle = if (title == "μλ₯") appliedTitle else null, + appliedTitle = if (index == 0) appliedTitle else null, ) }
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
β Files ignored due to path filters (10)
core/design-system/src/dev/res/drawable/img_bus.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_choice.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_home.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_model_student.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_night_bus.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_outing.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_percent.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_small_home.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_student_tag.pngis excluded by!**/*.pngcore/design-system/src/dev/res/drawable/img_volunteer.pngis excluded by!**/*.png
π Files selected for processing (33)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.ktcore/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.ktcore/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTab.ktcore/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTabRow.ktcore/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.ktcore/ui/src/dev/java/team/aliens/dms/android/core/ui/navigation/ResultStore.ktcore/ui/src/dev/java/team/aliens/dms/android/core/ui/util/Date.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/di/RepositoryModule.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/mapper/RemainsMapper.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/model/AppliedRemainsOption.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/model/RemainsApplicationTime.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/model/RemainsOption.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/repository/RemainsRepository.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/repository/RemainsRepositoryImpl.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/di/RepositoryModule.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/mapper/VotingMapper.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/model/AllVoteSearch.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/model/ModelStudentCandidates.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/model/StudentGcnInfo.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/model/Vote.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/model/VotingItem.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepository.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepositoryImpl.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationRoute.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/viewmodel/ApplicationViewModel.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/meal/ui/MealScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/remain/navigation/RemainApplicationRoute.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/component/DmsFloatingNotice.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt
π§° Additional context used
𧬠Code graph analysis (11)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/navigation/RemainApplicationRoute.kt (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (1)
RemainApplication(36-52)
data/src/dev/kotlin/team.aliens.dms.android.data/remain/model/AppliedRemainsOption.kt (1)
data/src/prod/kotlin/team/aliens/dms/android/data/remains/model/AppliedRemainsOption.kt (1)
id(5-8)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/viewmodel/ApplicationViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(29-31)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationRoute.kt (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (1)
Application(31-61)
data/src/dev/kotlin/team.aliens.dms.android.data/voting/di/RepositoryModule.kt (1)
data/src/prod/kotlin/team/aliens/dms/android/data/voting/di/RepositoryModule.kt (1)
bindVotingRepository(11-18)
data/src/dev/kotlin/team.aliens.dms.android.data/remain/di/RepositoryModule.kt (1)
data/src/prod/kotlin/team/aliens/dms/android/data/remains/di/RepositoryModule.kt (1)
bindRemainsRepository(11-18)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (4)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTabRow.kt (1)
DmsTabRow(16-55)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTab.kt (1)
DmsTab(14-44)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
ApplicationContent(14-44)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.kt (1)
VoteContent(17-49)
data/src/dev/kotlin/team.aliens.dms.android.data/remain/repository/RemainsRepositoryImpl.kt (1)
data/src/prod/kotlin/team/aliens/dms/android/data/remains/repository/RemainsRepositoryImpl.kt (1)
networkRemainsDataSource(11-27)
data/src/dev/kotlin/team.aliens.dms.android.data/remain/repository/RemainsRepository.kt (1)
data/src/prod/kotlin/team/aliens/dms/android/data/remains/repository/RemainsRepository.kt (1)
updateRemainsOption(8-17)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (4)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/component/DmsFloatingNotice.kt (1)
DmsFloatingNotice(29-65)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.kt (1)
DmsApplicationCard(33-116)feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (1)
setSelectRemainsOption(49-51)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
DmsButton(253-351)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(29-31)
β° 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). (1)
- GitHub Check: build
π Additional comments (27)
feature/src/dev/kotlin/team/aliens/dms/android/feature/meal/ui/MealScreen.kt (1)
17-18: LGTM! Clean scrolling implementation.The vertical scroll addition properly handles content overflow scenarios. The implementation follows Compose best practices using
rememberScrollState(), and the vertical scrolling gestures won't conflict with the horizontalHorizontalPagergestures.Also applies to: 151-152
data/src/dev/kotlin/team.aliens.dms.android.data/remain/di/RepositoryModule.kt (1)
1-18: Confirm package naming differs between dev and prod variants: "remain" vs "remains".The dev variant consistently uses
team.aliens.dms.android.data.remain.*(singular) across all feature packages (repository, model, mapper, di), while the prod variant usesteam.aliens.dms.android.data.remains.*(plural). This systematic difference should be verified as intentional, as other features maintain consistent naming across build variants.The Dagger Hilt module structure itself is correct and follows best practices.
data/src/dev/kotlin/team.aliens.dms.android.data/voting/model/StudentGcnInfo.kt (1)
1-6: The source set location is intentional and correct for this project structure.This is a multi-variant Android project using
src/prod/andsrc/dev/source sets, notsrc/main/. TheStudentGcnInfoclass exists in both the dev and prod variants as appropriate. The prod variant version is actively imported and used inVotingStudentScreen.ktandVotingModelStudentScreen.kt. This location is not a misconfiguration.Likely an incorrect or invalid review comment.
core/ui/src/dev/java/team/aliens/dms/android/core/ui/util/Date.kt (1)
1-7: Unused import and function with no critical issues.The
toDateString()function is not used in production code. WhileVoteContent.ktimports it on line 13, the actual code does not calltoDateString()βinstead usingtoLocalTime()on line 42. The import is unused. Since both this utility and its only importer are in dev source sets, having the file insrc/devis appropriate and intentional.data/src/dev/kotlin/team.aliens.dms.android.data/remain/model/AppliedRemainsOption.kt (1)
5-8: LGTM!Clean data model with appropriate types for the domain.
data/src/dev/kotlin/team.aliens.dms.android.data/remain/model/RemainsOption.kt (1)
5-10: LGTM!Well-structured data model with clear property semantics.
data/src/dev/kotlin/team.aliens.dms.android.data/remain/mapper/RemainsMapper.kt (1)
10-33: LGTM!Clean mapper implementation with appropriate visibility modifiers. The use of
internalfor public mappers andprivatefor the nested helper function follows good encapsulation practices.data/src/dev/kotlin/team.aliens.dms.android.data/remain/repository/RemainsRepositoryImpl.kt (1)
11-30: Implementation pattern is correct for dev variant, but confirms the dev/prod API incompatibility.The use of
runCatchingto wrap network calls aligns with theResult<T>return types in the dev abstract class. However, the prod version (shown in relevant code snippets) doesn't userunCatchingand returns values directly, confirming the critical API incompatibility already flagged inRemainsRepository.kt.The implementation itself is sound for the dev variant. Please address the dev/prod inconsistency flagged in the repository interface.
data/src/dev/kotlin/team.aliens.dms.android.data/voting/model/VotingItem.kt (1)
5-8: LGTM!Clean and minimal data class with appropriate types. The structure aligns well with the voting data layer requirements.
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTab.kt (1)
14-44: LGTM!Well-structured wrapper around Material3 Tab with appropriate theme integration. The destructuring for text style and color based on selection state is clean and readable.
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTabRow.kt (1)
16-55: LGTM!Well-implemented tab row wrapper with proper bounds checking for the indicator (line 24). The rounded corner styling on the indicator is a nice touch, and the theming integration is consistent.
core/ui/src/dev/java/team/aliens/dms/android/core/ui/navigation/ResultStore.kt (1)
79-84: Verify Saver works correctly across process death.The Saver saves
MutableState<Any?>objects, butMutableStateitself is notParcelable. This may work for configuration changes (where the instance is retained in memory) but could fail on process death when actual serialization is required.Ensure the values stored in
resultStateMapareParcelableorSerializable, or test process death scenarios to confirm restoration works correctly.data/src/dev/kotlin/team.aliens.dms.android.data/voting/model/Vote.kt (1)
3-6: LGTM!Clear and well-named enum constants for vote types.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.kt (1)
32-47: LGTM on the card rendering logic.The icon selection based on
voteTypeusingwhenexpression is clean and exhaustive. The integration withDmsApplicationCardfollows the expected API pattern.data/src/dev/kotlin/team.aliens.dms.android.data/voting/model/AllVoteSearch.kt (1)
6-14: LGTM!Clean data class definition with appropriate types. Using
java.time.LocalDateTimeis the recommended approach for date-time handling in modern Kotlin/Android.data/src/dev/kotlin/team.aliens.dms.android.data/voting/di/RepositoryModule.kt (1)
11-18: LGTM!Standard Hilt module binding pattern, consistent with the existing
prodvariant and other repository modules in the codebase.feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/navigation/RemainApplicationRoute.kt (1)
7-16: LGTM!Clean route wrapper that properly delegates to the internal
RemainApplicationcomposable, maintaining good separation between navigation routing and screen implementation.core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.kt (1)
33-116: LGTM!Well-structured reusable card component with appropriate theming and layout. The conditional
appliedTitledisplay logic (header vs. description row) is clear and the animated border state provides good visual feedback.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/viewmodel/ApplicationViewModel.kt (1)
57-60: LGTM on the state design.Clean state class with sensible defaults. Keeping this as a simple data class is appropriate.
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (2)
36-52: LGTM!The entry point composable correctly obtains the ViewModel via
hiltViewModel(), collects state with lifecycle awareness, and properly wires the callbacks to the screen composable.
54-118: LGTM!The screen composable is well-structured with proper separation of concerns. The UI layout with top app bar, floating notice, lazy column, and bottom button follows a clean pattern. The button is correctly enabled only when a selection exists.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationRoute.kt (1)
9-24: LGTM!The route function correctly serves as a thin wrapper that forwards all navigation and snackbar callbacks to the
Applicationcomposable. This follows a clean separation pattern between navigation wiring and UI logic.data/src/dev/kotlin/team.aliens.dms.android.data/voting/mapper/VotingMapper.kt (2)
16-25: Potential runtime exceptions in mapping.
UUID.fromString()(line 18) andVote.valueOf()(line 23) can throwIllegalArgumentExceptionif the backend returns an invalid UUID or an unrecognized vote type. While these exceptions will be caught byrunCatchingin the repository layer, the error message won't be descriptive.Consider adding defensive checks or using
runCatchingwithin the mapper for more granular error handling if API contracts are not guaranteed.
27-45: LGTM!The mapping functions for
VotingItemandModelStudentCandidatesare straightforward field-to-field mappings with no transformation risks.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (1)
63-128: LGTM!The
ApplicationScreencomposable is well-structured with a clean pager implementation. The tab synchronization withcoroutineScope.launchandanimateScrollToPagefollows the recommended pattern. UsingbeyondViewportPageCount = 1is good for preloading adjacent pages.data/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepositoryImpl.kt (1)
12-34: LGTM!The repository implementation is clean and consistent:
- Proper use of
@Injectfor DI- All operations wrapped in
runCatchingfor error handling- Consistent use of
toModel()mappers for response transformationinternalvisibility is appropriate for DI module bindingNote: The naming issues mentioned in
VotingRepository.ktapply here as well (fetchCreateVotingItemandfetchDeleteVotingItemshould use create/delete prefixes).feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
14-21: LGTM!The function signature follows Compose best practices with proper modifier-first parameter ordering and clear navigation callback names.
core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt
Show resolved
Hide resolved
| inline fun <reified T> getResultState(resultKey: String = T::class.toString()) = | ||
| resultStateMap[resultKey]?.value as T |
There was a problem hiding this comment.
Unsafe cast can cause NPE or ClassCastException.
When resultKey doesn't exist in the map, resultStateMap[resultKey]?.value returns null, and casting null as T will fail for non-nullable types. This should return a nullable type or handle the missing key case.
π Proposed fix
- inline fun <reified T> getResultState(resultKey: String = T::class.toString()) =
- resultStateMap[resultKey]?.value as T
+ inline fun <reified T> getResultState(resultKey: String = T::class.toString()): T? =
+ resultStateMap[resultKey]?.value as? TAlternatively, if non-null is required, provide a version that throws with a clear message:
inline fun <reified T> getResultStateOrThrow(resultKey: String = T::class.toString()): T =
resultStateMap[resultKey]?.value as? T
?: error("No result found for key: $resultKey")π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline fun <reified T> getResultState(resultKey: String = T::class.toString()) = | |
| resultStateMap[resultKey]?.value as T | |
| inline fun <reified T> getResultState(resultKey: String = T::class.toString()): T? = | |
| resultStateMap[resultKey]?.value as? T |
π€ Prompt for AI Agents
In
core/ui/src/dev/java/team/aliens/dms/android/core/ui/navigation/ResultStore.kt
around lines 61-62, the inline getResultState currently performs an unsafe cast
which can throw when the key is missing or the type mismatches; change the
function to return a nullable T using a safe cast (as?) so it returns null when
no value or wrong type is present, or alternatively add a new
getResultStateOrThrow that uses a safe cast and throws a clear error if the
result is absent; ensure the function signature reflects nullability for the
nullable variant and update call sites accordingly.
data/src/dev/kotlin/team.aliens.dms.android.data/remain/repository/RemainsRepository.kt
Show resolved
Hide resolved
| ApplicationScreen( | ||
| state = state, | ||
| onNavigateRemainApplication = onNavigateRemainApplication, | ||
| onNavigateOutingApplication = { onShowSnackBar(DmsSnackBarType.SUCCESS, "μ€λΉμ€μΈ κΈ°λ₯μ΄μμ") }, | ||
| onNavigateVolunteerApplication = { onShowSnackBar(DmsSnackBarType.SUCCESS, "μ€λΉμ€μΈ κΈ°λ₯μ΄μμ") }, | ||
| onNavigateVote = onNavigateVote, | ||
| ) |
There was a problem hiding this comment.
Unused navigation callbacks replaced with snackbar messages.
onNavigateOutingApplication and onNavigateVolunteerApplication are accepted as parameters (lines 34-35) but are never used. Instead, they're replaced with snackbar messages indicating "μ€λΉμ€μΈ κΈ°λ₯μ΄μμ" (feature in preparation).
If these features are not yet implemented, consider either:
- Not accepting these callbacks as parameters and hardcoding the snackbar behavior
- Using the passed callbacks as intended once the features are ready
This creates a misleading API where callers provide callbacks that are silently ignored.
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/component/DmsFloatingNotice.kt
Show resolved
Hide resolved
| class RemainApplicationViewModel @Inject constructor( | ||
| val remainsRepository: RemainsRepository | ||
| ): BaseStateViewModel<RemainApplicationState, Unit>(RemainApplicationState()) { |
There was a problem hiding this comment.
π οΈ Refactor suggestion | π Major
Make remainsRepository private.
The repository should not be exposed publicly from the ViewModel. This violates encapsulation and allows external code to bypass ViewModel logic.
π Proposed fix
@HiltViewModel
class RemainApplicationViewModel @Inject constructor(
- val remainsRepository: RemainsRepository
+ private val remainsRepository: RemainsRepository
): BaseStateViewModel<RemainApplicationState, Unit>(RemainApplicationState()) {π€ Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt
around lines 18-20, the injected remainsRepository is declared public which
exposes the repository outside the ViewModel; make it private by changing its
visibility to private (e.g., private val remainsRepository: RemainsRepository)
so external code cannot access it directly and the ViewModel retains
encapsulation while leaving constructor injection intact.
...rc/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt
Show resolved
Hide resolved
...rc/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt
Outdated
Show resolved
Hide resolved
| if (startDayValue == endDayValue) { | ||
| return currentDayOfWeek == startDayValue && | ||
| currentTime >= startTime && currentTime <= endTime | ||
| } | ||
|
|
||
| return when { | ||
| currentDayOfWeek == startDayValue -> currentTime >= startTime | ||
| currentDayOfWeek == endDayValue -> currentTime <= endTime | ||
| currentDayOfWeek in (startDayValue + 1)..<endDayValue -> true | ||
| else -> false | ||
| } |
There was a problem hiding this comment.
Week wraparound not handled in time validation.
The range check currentDayOfWeek in (startDayValue + 1)..<endDayValue fails when the application period spans across week boundaries (e.g., Friday to Monday). In such cases, startDayValue (5) > endDayValue (1), making the range empty.
π Proposed fix for week wraparound
if (startDayValue == endDayValue) {
return currentDayOfWeek == startDayValue &&
currentTime >= startTime && currentTime <= endTime
}
- return when {
- currentDayOfWeek == startDayValue -> currentTime >= startTime
- currentDayOfWeek == endDayValue -> currentTime <= endTime
- currentDayOfWeek in (startDayValue + 1)..<endDayValue -> true
- else -> false
+ // Handle week wraparound (e.g., Friday to Monday)
+ val isWithinDayRange = if (startDayValue < endDayValue) {
+ currentDayOfWeek in startDayValue..endDayValue
+ } else {
+ currentDayOfWeek >= startDayValue || currentDayOfWeek <= endDayValue
+ }
+
+ if (!isWithinDayRange) return false
+
+ return when (currentDayOfWeek) {
+ startDayValue -> currentTime >= startTime
+ endDayValue -> currentTime <= endTime
+ else -> true
}π€ Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt
around lines 102 to 112, the weekday range check doesn't handle week wraparound
(when startDayValue > endDayValue), so the expression `currentDayOfWeek in
(startDayValue + 1)..<endDayValue` becomes empty; change the logic to detect
wraparound and, when startDayValue <= endDayValue keep the existing middle-range
check, but when startDayValue > endDayValue treat middle days as those >=
startDayValue OR <= endDayValue (i.e., use an OR condition to cover the
end-of-week continuation), ensuring the start-day and end-day branches remain
the same and preserve time comparisons for boundary days.
π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
π€ Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 11
β»οΈ Duplicate comments (9)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (5)
18-20: Make repository private.The injected repository should not be publicly accessible from the ViewModel, as this breaks encapsulation.
This issue was already flagged in previous reviews.
29-31: Avoid random UUID for missing selection.Generating a random UUID when no option is applied creates a meaningless identifier. The state property
selectRemainsOptionIdis already nullable (line 118), so usenullhere.This issue was already flagged in previous reviews.
61-62: Critical: Invalid UUID sent to server.When
selectRemainsOptionIdis null, this code generates a random UUID and sends it to the server, which will fail or cause unintended behavior. Add an early return with error feedback.This critical issue was already flagged in previous reviews.
64-79: Refactor to eliminate side effects in map.Calling
setStateinside themapfunction (line 66) is an anti-pattern that causes side effects during iteration. The subsequentsetStateat lines 73-78 is also redundant. Consolidate into a single atomic state update.This issue was already flagged in previous reviews.
107-112: Week wraparound not handled.The range check fails when the application period spans week boundaries (e.g., Friday to Monday), because
startDayValue>endDayValuemakes the range on line 110 empty.This issue was already flagged in previous reviews with a detailed fix.
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
30-41: Hardcoded strings and magic string comparison.The use of
listOf(Triple(...))with hardcoded Korean strings and the magic string comparisonif (title == "μλ₯")reduces maintainability. This concern was already raised in a previous review suggesting a data class approach.app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
3-3: Unused import.
android.util.Logis imported but not used in this file.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (2)
44-44: Define a constant for the result key.The magic string
"remain_application_result"is hardcoded here and inDmsApp.kt. This was previously flagged.
56-57: Unused navigation callbacks replaced with snackbar messages.
onNavigateOutingApplicationandonNavigateVolunteerApplicationparameters (lines 33-34) are accepted but replaced with snackbar messages. This was previously flagged.
π§Ή Nitpick comments (11)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt (1)
49-80: Remove commented-out logging statements.The commented-out
Logger.a(...)calls on lines 54, 65, and 76 should either be removed or properly implemented. Leaving dead code reduces readability.core/ui/src/dev/java/team/aliens/dms/android/core/ui/util/Date.kt (1)
5-7: Consider usingDateTimeFormatterfor consistent formatting.The current implementation lacks zero-padding for single-digit months/days (e.g., "1μ 5μΌ" vs "01μ 05μΌ"). If consistent padding is desired, consider using
DateTimeFormatter:π Alternative using DateTimeFormatter
import java.time.format.DateTimeFormatter private val koreanDateFormatter = DateTimeFormatter.ofPattern("yyyyλ MMμ ddμΌ") fun LocalDateTime.toDateString(): String = format(koreanDateFormatter)feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/VoteItemContent.kt (1)
24-70: Consider removing unnecessaryColumnwrapper.The outer
Columnwraps awhenexpression where each branch renders exactly one composable. The wrapper adds an unnecessary layout layer and doesn't provide additional structure.π Simplified implementation
- Column( - modifier = modifier, - ) { - when (vote) { - Vote.OPTION_VOTE -> { - OptionContent( - title = title, - startTime = startTime, - endTime = endTime, - options = options, - selectItem = selectItem, - onSelect = onSelect, - ) - } - // ... other cases - } - } + when (vote) { + Vote.OPTION_VOTE -> OptionContent( + modifier = modifier, + title = title, + startTime = startTime, + endTime = endTime, + options = options, + selectItem = selectItem, + onSelect = onSelect, + ) + Vote.STUDENT_VOTE -> StudentContent( + modifier = modifier, + title = title, + startTime = startTime, + endTime = endTime, + students = students, + selectItem = selectItem, + onSelect = onSelect, + ) + Vote.APPROVAL_VOTE -> ApprovalContent( + modifier = modifier, + title = title, + options = options, + selectItem = selectItem, + onSelect = onSelect, + ) + Vote.MODEL_STUDENT_VOTE -> StudentContent( + modifier = modifier, + title = title, + startTime = startTime, + endTime = endTime, + students = modelStudents, + selectItem = selectItem, + onSelect = onSelect, + ) + }feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (2)
3-3: Unused import.
android.util.Logis imported but not used in this file.π Proposed fix
-import android.util.Log
41-66: Side effect collection insideLaunchedEffect(Unit)may miss emissions.The
sideEffectcollection starts afterinitState()completes. IfinitStateemits a side effect synchronously, it could be missed. Consider separating the initialization and side effect collection into differentLaunchedEffectblocks.π Proposed fix
- LaunchedEffect(Unit) { - viewModel.initState(title, startTime, endTime) - viewModel.sideEffect.collect { + LaunchedEffect(Unit) { + viewModel.sideEffect.collect { when (it) { is VoteSideEffect.VoteSuccess -> onShowSnackBar( DmsSnackBarType.SUCCESS, "ν¬νλ₯Ό μλ£νμ΄μ!", ) // ... other cases } } } + + LaunchedEffect(Unit) { + viewModel.initState(title, startTime, endTime) + }feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/StudentContent.kt (2)
93-106: Consider memoizing filtered students to avoid repeated computation.
filteredStudentsis recalculated on every recomposition. Usingrememberwith appropriate keys would improve performance, especially for large student lists.π Proposed fix
- val selectGrade = page + 1 - LazyColumn(modifier = Modifier.fillMaxSize()) { - val filteredStudents = students.filter { it.gradeClassNumber.startsWith("$selectGrade") } + val selectGrade = page + 1 + val filteredStudents = remember(students, selectGrade) { + students.filter { it.gradeClassNumber.startsWith("$selectGrade") } + } + LazyColumn(modifier = Modifier.fillMaxSize()) {
137-146: Add placeholder and error handling forAsyncImage.If
profileImageUrlis empty, null, or fails to load, the image area will be blank. Consider adding placeholder and error drawables for better UX.π Proposed fix
AsyncImage( modifier = Modifier .size(32.dp) .clip(CircleShape), model = ImageRequest.Builder(context = LocalContext.current) .data(profileImageUrl) + .crossfade(true) .build(), contentDescription = null, contentScale = ContentScale.Crop, + placeholder = painterResource(R.drawable.ic_default_profile), // Add appropriate drawable + error = painterResource(R.drawable.ic_default_profile), )app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (2)
169-178: Navigation back creates newVoteScreenNavinstance for removal.The back navigation creates a new
VoteScreenNav(voteNav.title, voteNav.startTime, voteNav.endTime)to remove from the backstack. While data class equality should make this work, consider usingbackStack.removeLastOrNull()for consistency with other navigation patterns, or directly reference thevoteNavinstance.π Proposed fix
- onNavigateBack = { backStack.remove(VoteScreenNav(voteNav.title, voteNav.startTime, voteNav.endTime)) }, + onNavigateBack = { backStack.removeLastOrNull() },
180-189: Define a constant for the result key"remain_application_result".This magic string is used here and in
ApplicationScreen.kt. Define a shared constant to prevent typos and improve maintainability.// In a shared constants file or companion object const val REMAIN_APPLICATION_RESULT_KEY = "remain_application_result"feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/ApprovalContent.kt (1)
1-2: Remove leading blank line.There's an unnecessary blank line before the package declaration.
π Proposed fix
- package team.aliens.dms.android.feature.vote.ui.componentfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (1)
42-51: Consider null-safety when casting result.The cast
as? String?with nullable chaining is correct, but thesnapshotFlowwill emit on every read ofresultStateMap, potentially causing unnecessary collections. Consider usingdistinctUntilChanged().π Proposed fix
LaunchedEffect(Unit) { snapshotFlow { resultStore.resultStateMap["remain_application_result"]?.value as? String? - }.collect { result -> + }.distinctUntilChanged().collect { result -> if (result != null) { viewModel.setRemainApplication(result) resultStore.removeResult<String?>(resultKey = "remain_application_result") } } }Add the import:
import kotlinx.coroutines.flow.distinctUntilChanged
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (23)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.ktcore/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/modifier/DmsShadowModifier.ktcore/design-system/src/dev/res/drawable/ic_approve.xmlcore/design-system/src/dev/res/drawable/ic_oppose.xmlcore/ui/src/dev/java/team/aliens/dms/android/core/ui/util/Date.ktdata/build.gradle.ktsdata/src/dev/kotlin/team.aliens.dms.android.data/voting/mapper/VotingMapper.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/model/AllVoteSearch.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepository.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepositoryImpl.ktfeature/build.gradle.ktsfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/ApprovalContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/OptionContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/StudentContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/TitleContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/VoteItemContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt
π§ Files skipped from review as they are similar to previous changes (3)
- data/src/dev/kotlin/team.aliens.dms.android.data/voting/mapper/VotingMapper.kt
- data/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepository.kt
- feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.kt
π§° Additional context used
𧬠Code graph analysis (10)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/VoteItemContent.kt (3)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/OptionContent.kt (1)
OptionContent(27-63)feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/StudentContent.kt (1)
StudentContent(41-110)feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/ApprovalContent.kt (1)
ApprovalContent(34-96)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/VoteItemContent.kt (1)
VoteItemContent(11-71)
data/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepositoryImpl.kt (1)
data/src/prod/kotlin/team/aliens/dms/android/data/voting/repository/VotingRepositoryImpl.kt (1)
networkVotingDataSource(12-31)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/OptionContent.kt (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/TitleContent.kt (1)
TitleContent(17-41)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/StudentContent.kt (3)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/TitleContent.kt (1)
TitleContent(17-41)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTabRow.kt (1)
DmsTabRow(16-55)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTab.kt (1)
DmsTab(14-44)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (4)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTabRow.kt (1)
DmsTabRow(16-55)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/tab/DmsTab.kt (1)
DmsTab(14-44)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
ApplicationContent(13-43)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.kt (1)
VoteContent(16-48)
feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.kt (1)
DmsApplicationCard(33-116)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(29-31)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (1)
Vote(30-74)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (2)
setState(29-31)sendEffect(33-35)
β° 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). (1)
- GitHub Check: build
π Additional comments (15)
core/design-system/src/dev/res/drawable/ic_approve.xml (1)
1-11: No action needed. Theic_approvedrawable is correctly designed for its intended use as a selectable voting button in the approval voting UI (ApprovalItemcomponent). The circle outline, which changes color on selection, is semantically appropriateβit represents the "approve" option in the voting interface, withic_oppose(an X symbol) as its counterpart. The naming and design are intentional and correct.feature/build.gradle.kts (1)
73-74: No action required. Both network module dependencies serve distinct purposes and are actively used by the feature module. TheProjectPaths.Core.NETWORKdependency provides core exception types (e.g.,ConflictException), whileProjectPaths.NETWORKprovides feature-specific API services and build configuration. The dependency structure is correct.feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (1)
93-95: Verify permissive default when time window is unconfigured.When
startTimeorendTimeis empty, the function returnstrue, allowing applications at any time. Confirm this is the intended behavior rather than requiring a configured time window.data/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepositoryImpl.kt (1)
15-33: The dev implementation is correct and matches its interface definition.The dev
VotingRepositoryinterface declares all methods withResult<T>return types, and theVotingRepositoryImplimplementation correctly provides those return types usingrunCatching. This is not an inconsistencyβthe dev and prod flavors intentionally have different interface definitions:
- Dev interface:
Result<List<AllVoteSearch>>,Result<List<VotingItem>>,Result<Unit>- Dev implementation: Matches the dev interface β
- Prod interface:
List<AllVoteSearch>,List<VotingItem>,Unit(no Result wrapper)- Prod implementation: Matches the prod interface β
Each flavor's implementation correctly implements its own interface, so there is no compilation failure or error-handling inconsistency. The different return types between flavors are an intentional design choice.
Likely an incorrect or invalid review comment.
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/modifier/DmsShadowModifier.kt (1)
58-59: LGTM!The enum provides a clean API for selecting shadow presets with meaningful names indicating intensity levels.
core/design-system/src/dev/res/drawable/ic_oppose.xml (1)
1-9: LGTM!The vector drawable is well-structured. The X-shaped path appropriately represents an "oppose" action. Consider using a theme color attribute (e.g.,
?attr/colorError) instead of the hardcoded#FE0F0Ffor better dark/light theme consistency, but this is optional for icon resources.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt (1)
8-23: LGTM!The route wrapper follows a clean pattern for separating navigation concerns from screen implementation.
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/TitleContent.kt (1)
17-41: LGTM!Clean component implementation with proper theming and consistent layout structure. The date range display using
toDateString()is clear and readable.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
13-43: LGTM on the component structure.The composable correctly wires navigation callbacks to each card and properly forwards
appliedTitleonly to the "μλ₯" card. The layout with spacing and padding is well-structured.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (1)
76-123: LGTM!The
VoteScreencomposable is well-structured with proper state-driven UI, including loading and enabled states for the submit button.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/StudentContent.kt (1)
41-110: Well-structured tabbed student selector.The implementation correctly uses
HorizontalPagerwithDmsTabRowsynchronization via coroutine scope. The grade filtering and student selection logic is clear.app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
116-200: Good integration of result store with composition local.The
CompositionLocalProviderpattern for sharingresultStoreacross navigation destinations is well-implemented, enabling cross-destination result passing.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/ApprovalContent.kt (1)
67-93: Hardcoded approve/oppose logic assumes option ordering.The first option is always treated as "approve" and subsequent options as "oppose". This is fragile if the backend order changes. Consider using a property on
VotingItemto determine the option type, or at minimum, add a comment documenting this assumption.feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/OptionContent.kt (1)
65-101: LGTM!The
OptionItemcomposable follows a consistent pattern withStudentItem, providing good code uniformity across the voting UI components.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt (1)
62-127: Well-structured tabbed application screen.The
ApplicationScreenproperly implements a two-tab layout withHorizontalPagerand correctly passes state and callbacks to child components.
| @Composable | ||
| fun Modifier.dmsShadowModifier( | ||
| dmsShadowType: DmsShadowType, | ||
| shape: Shape = CircleShape, | ||
| ): Modifier { |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
# First, let's find where dmsDropShadow is defined
fd -t f "\.kt$" -x rg -l "fun.*dmsDropShadow" {}Repository: team-aliens/DMS-Android
Length of output: 168
π Script executed:
# Also check the full implementation of dmsShadowModifier
cat core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/modifier/DmsShadowModifier.ktRepository: team-aliens/DMS-Android
Length of output: 1750
π Script executed:
cat core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/modifier/DmsDropShadow.ktRepository: team-aliens/DMS-Android
Length of output: 1382
Remove the @Composable annotation.
The function only delegates to dmsDropShadow, which is not a composable function and uses drawBehind under the hood. The @Composable annotation is unnecessary and adds overhead.
Additionally, CircleShape as the default is inappropriate for most use casesβshadows are typically applied to cards and buttons with rounded corners, not circular components. Consider removing the default or using a more universally applicable shape.
π€ Prompt for AI Agents
In
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/modifier/DmsShadowModifier.kt
around lines 10 to 14, remove the unnecessary @Composable annotation from the
dmsShadowModifier function (since it only delegates to non-composable
dmsDropShadow which uses drawBehind) and change the default shape from
CircleShape to a more general RoundedCornerShape (or remove the default
entirely) by updating the function signature accordingly so itβs a plain
extension function returning Modifier and uses a neutral/default rounded corner
shape if a default is required.
| alias(libs.plugins.hilt) | ||
| alias(libs.plugins.ksp) | ||
| alias(libs.plugins.ktlint.gradle) | ||
| alias(libs.plugins.serialization) |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
rg -n --type=kt '@Serializable' data/src/Repository: team-aliens/DMS-Android
Length of output: 93
π Script executed:
rg '@Serializable' data/src/ --include='*.kt'Repository: team-aliens/DMS-Android
Length of output: 508
π Script executed:
rg '@Serializable' data/src/Repository: team-aliens/DMS-Android
Length of output: 49
π Script executed:
fd . data/src/ --type f -e kt && rg -i 'serializ|json|moshi|gson' data/src/Repository: team-aliens/DMS-Android
Length of output: 12022
Remove the unused serialization plugin.
The Kotlin serialization plugin was added but is not used anywhere in the data module. No @Serializable annotations or serialization-related imports were found across the data models. Remove alias(libs.plugins.serialization) from the build configuration unless it will be actively used.
π€ Prompt for AI Agents
In data/build.gradle.kts around line 10, the Kotlin serialization plugin is
declared but unused; remove the line alias(libs.plugins.serialization) from the
file to avoid including an unnecessary plugin and keep the build configuration
clean. Ensure no other references to the serialization plugin remain in the data
module (build scripts or imports); if later needed, re-add the plugin where
serialization annotations/usages exist.
| data class AllVoteSearch( | ||
| val id: UUID = UUID.randomUUID(), | ||
| val topicName: String = "", | ||
| val description: String = "", | ||
| val startTime: LocalDateTime = now, | ||
| val endTime: LocalDateTime = now, | ||
| val voteType: Vote = Vote.STUDENT_VOTE, | ||
| val isVoted: Boolean = false, | ||
| ) |
There was a problem hiding this comment.
Remove or reconsider the UUID.randomUUID() default parameter.
Using UUID.randomUUID() as a default value for id is problematic because it generates a new random UUID for every instance created with the default. This breaks expected data class behavior:
val vote1 = AllVoteSearch()
val vote2 = AllVoteSearch()
// vote1 != vote2, even though all other fields are identicalThis undermines equality checks and can cause subtle bugs in collections, caching, or comparison logic.
π Recommended fix
Consider one of these alternatives:
Option 1: Remove the default (preferred if ID should always be provided)
data class AllVoteSearch(
- val id: UUID = UUID.randomUUID(),
+ val id: UUID,
val topicName: String = "",
val description: String = "",
val startTime: LocalDateTime = now,
val endTime: LocalDateTime = now,
val voteType: Vote = Vote.STUDENT_VOTE,
val isVoted: Boolean = false,
)Option 2: Use a sentinel value for "unassigned"
data class AllVoteSearch(
- val id: UUID = UUID.randomUUID(),
+ val id: UUID = UUID.fromString("00000000-0000-0000-0000-000000000000"),
val topicName: String = "",
val description: String = "",
val startTime: LocalDateTime = now,
val endTime: LocalDateTime = now,
val voteType: Vote = Vote.STUDENT_VOTE,
val isVoted: Boolean = false,
)π€ Prompt for AI Agents
In
data/src/dev/kotlin/team.aliens.dms.android.data/voting/model/AllVoteSearch.kt
around lines 7 to 15, the default UUID.randomUUID() for the id field causes
every default-constructed AllVoteSearch to have a different id which breaks
equality and collection behavior; either remove the default so callers must
supply an id (preferred), or replace it with a stable sentinel (e.g., a fixed
ZERO UUID constant or nullable id) to represent "unassigned" and update
usages/tests accordingly to construct explicit ids when needed.
| val startTime = LocalTime.parse(applicationTime.startTime) | ||
| val endTime = LocalTime.parse(applicationTime.endTime) |
There was a problem hiding this comment.
Add error handling for time parsing.
LocalTime.parse() will throw DateTimeParseException if the time strings from the repository are malformed. Wrap in a try-catch or validate format before parsing.
π Proposed fix
- val startTime = LocalTime.parse(applicationTime.startTime)
- val endTime = LocalTime.parse(applicationTime.endTime)
+ val startTime = try {
+ LocalTime.parse(applicationTime.startTime)
+ } catch (e: Exception) {
+ return false
+ }
+ val endTime = try {
+ LocalTime.parse(applicationTime.endTime)
+ } catch (e: Exception) {
+ return false
+ }π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val startTime = LocalTime.parse(applicationTime.startTime) | |
| val endTime = LocalTime.parse(applicationTime.endTime) | |
| val startTime = try { | |
| LocalTime.parse(applicationTime.startTime) | |
| } catch (e: Exception) { | |
| return false | |
| } | |
| val endTime = try { | |
| LocalTime.parse(applicationTime.endTime) | |
| } catch (e: Exception) { | |
| return false | |
| } |
| import androidx.compose.runtime.Composable | ||
| import team.aliens.dms.android.core.designsystem.snackbar.DmsSnackBarType | ||
| import team.aliens.dms.android.feature.vote.ui.Vote | ||
| import java.time.LocalDateTime |
There was a problem hiding this comment.
Remove unused import.
java.time.LocalDateTime is imported but not used in this file.
π Proposed fix
import team.aliens.dms.android.core.designsystem.snackbar.DmsSnackBarType
import team.aliens.dms.android.feature.vote.ui.Vote
-import java.time.LocalDateTimeπ Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import java.time.LocalDateTime | |
| import team.aliens.dms.android.core.designsystem.snackbar.DmsSnackBarType | |
| import team.aliens.dms.android.feature.vote.ui.Vote |
π€ Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt
around line 6, the import java.time.LocalDateTime is unused; remove that import
line to clean up the file and avoid unused-import warnings, ensuring no other
code depends on LocalDateTime in this file before committing the change.
| LazyColumn { | ||
| items( | ||
| items = options, | ||
| key = { it.id }, | ||
| ) { option -> | ||
| OptionItem( | ||
| title = option.votingOptionName, | ||
| selected = option.id.toString() == selectItem, | ||
| onClick = { onSelect(option.id.toString()) }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
LazyColumn lacks height constraints.
The LazyColumn doesn't have a Modifier.weight(1f) or fillMaxSize(). Inside a Column, this could cause layout issues where the list doesn't fill available space or becomes unbounded.
π Proposed fix
- LazyColumn {
+ LazyColumn(modifier = Modifier.weight(1f)) {
items(
items = options,
key = { it.id },π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LazyColumn { | |
| items( | |
| items = options, | |
| key = { it.id }, | |
| ) { option -> | |
| OptionItem( | |
| title = option.votingOptionName, | |
| selected = option.id.toString() == selectItem, | |
| onClick = { onSelect(option.id.toString()) }, | |
| ) | |
| } | |
| } | |
| LazyColumn(modifier = Modifier.weight(1f)) { | |
| items( | |
| items = options, | |
| key = { it.id }, | |
| ) { option -> | |
| OptionItem( | |
| title = option.votingOptionName, | |
| selected = option.id.toString() == selectItem, | |
| onClick = { onSelect(option.id.toString()) }, | |
| ) | |
| } | |
| } |
π€ Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/OptionContent.kt
around lines 50 to 61, the LazyColumn has no height constraints which can cause
unbounded or incorrect layout inside a parent Column; update the LazyColumn call
to pass a Modifier that constrains its height (for example Modifier.weight(1f)
when inside a Column or Modifier.fillMaxSize()/fillMaxHeight() as appropriate),
ensuring the Modifier is imported and applied to the LazyColumn so the list
fills the available space.
| VoteScreen( | ||
| state = state, | ||
| onNavigateBack = onNavigateBack, | ||
| onSelectItem = { selectedId -> viewModel.setSelectId(UUID.fromString(selectedId)) }, |
There was a problem hiding this comment.
UUID.fromString can throw IllegalArgumentException on invalid input.
If selectedId is not a valid UUID string, this line will crash. Consider adding validation or error handling.
π Proposed fix
- onSelectItem = { selectedId -> viewModel.setSelectId(UUID.fromString(selectedId)) },
+ onSelectItem = { selectedId ->
+ runCatching { UUID.fromString(selectedId) }
+ .onSuccess { viewModel.setSelectId(it) }
+ },π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onSelectItem = { selectedId -> viewModel.setSelectId(UUID.fromString(selectedId)) }, | |
| onSelectItem = { selectedId -> | |
| runCatching { UUID.fromString(selectedId) } | |
| .onSuccess { viewModel.setSelectId(it) } | |
| }, |
π€ Prompt for AI Agents
In feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt
around line 71, the call UUID.fromString(selectedId) can throw
IllegalArgumentException for invalid input; validate or guard the conversion and
only call viewModel.setSelectId when the string is a valid UUID (e.g., use a
try/catch or runCatching to parse, or pre-validate with a regex/UUID.parse
helper), and on failure either ignore the selection or log/report the invalid
value instead of letting the exception crash the UI.
| init { | ||
| fetchVotesByType() | ||
| } |
There was a problem hiding this comment.
Potential timing issue: fetchVotesByType() called before state initialization.
The init block calls fetchVotesByType() which reads uiState.value.vote.voteType. At this point, the state contains the default AllVoteSearch() with its default voteType. If the actual vote type is set via initState() later, this fetch happens with the wrong type.
Consider deferring the fetch until after initState() is called, or passing the vote type as a constructor parameter.
π€ Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt
around lines 28-30, the init block calls fetchVotesByType() too early (it reads
uiState.value.vote.voteType before initState() may set the intended value);
remove the call from init and instead invoke fetchVotesByType() from initState()
after you update uiState.value (or alternatively accept the desired voteType as
a constructor parameter and use that when initializing), ensuring the fetch runs
only after the uiState has been set to the correct voteType (and if needed
dispatch it on the main/VM scope to maintain thread-safety).
| private fun setButtonEnabled() { | ||
| val isSelectIdNotNull = uiState.value.selectId != null | ||
| setState { uiState.value.copy(buttonEnabled = isSelectIdNotNull) } | ||
| } |
There was a problem hiding this comment.
Incorrect setState usage causes potential race condition.
Using uiState.value.copy() inside setState lambda reads stale state. The lambda parameter it represents the current state and should be used instead.
π Proposed fix
private fun setButtonEnabled() {
- val isSelectIdNotNull = uiState.value.selectId != null
- setState { uiState.value.copy(buttonEnabled = isSelectIdNotNull) }
+ setState { it.copy(buttonEnabled = it.selectId != null) }
}π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun setButtonEnabled() { | |
| val isSelectIdNotNull = uiState.value.selectId != null | |
| setState { uiState.value.copy(buttonEnabled = isSelectIdNotNull) } | |
| } | |
| private fun setButtonEnabled() { | |
| setState { it.copy(buttonEnabled = it.selectId != null) } | |
| } |
π€ Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt
around lines 87 to 90, replace the current setState usage that reads
uiState.value inside the lambda with a version that uses the lambda parameter;
specifically, change setState { uiState.value.copy(buttonEnabled =
uiState.value.selectId != null) } to use the provided state variable (it) so:
setState { it.copy(buttonEnabled = it.selectId != null) } to avoid reading stale
state.
| internal fun postVote() { | ||
| with(uiState.value) { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| setState { uiState.value.copy(isLoading = true, buttonEnabled = false) } | ||
| voteRepository.fetchCreateVotingItem( | ||
| votingTopicId = uiState.value.vote.id, | ||
| selectedId = uiState.value.selectId ?: UUID.randomUUID(), | ||
| ).onSuccess { | ||
| setState { uiState.value.copy(buttonEnabled = false, isLoading = false) } | ||
| sendEffect(VoteSideEffect.VoteSuccess) | ||
| }.onFailure { | ||
| setState { uiState.value.copy(isLoading = false, buttonEnabled = true) } | ||
| // Logger.a(it) { it.message.toString() } | ||
| when (it) { | ||
| is ConflictException -> sendEffect(VoteSideEffect.VoteConflict) | ||
| else -> sendEffect(VoteSideEffect.VoteFail) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Multiple issues in postVote function.
with(uiState.value)on line 93 is unusedβthe receiver is never referenced inside the block.- Lines 95, 100, 103 use
uiState.value.copy()insidesetStateβshould useit.copy()to avoid race conditions. - Line 98:
UUID.randomUUID()as a fallback for nullselectIdcould submit invalid vote data. The vote should not proceed if no selection was made.
π Proposed fix
internal fun postVote() {
- with(uiState.value) {
- viewModelScope.launch(Dispatchers.IO) {
- setState { uiState.value.copy(isLoading = true, buttonEnabled = false) }
- voteRepository.fetchCreateVotingItem(
- votingTopicId = uiState.value.vote.id,
- selectedId = uiState.value.selectId ?: UUID.randomUUID(),
- ).onSuccess {
- setState { uiState.value.copy(buttonEnabled = false, isLoading = false) }
- sendEffect(VoteSideEffect.VoteSuccess)
- }.onFailure {
- setState { uiState.value.copy(isLoading = false, buttonEnabled = true) }
-// Logger.a(it) { it.message.toString() }
- when (it) {
- is ConflictException -> sendEffect(VoteSideEffect.VoteConflict)
- else -> sendEffect(VoteSideEffect.VoteFail)
- }
+ val currentState = uiState.value
+ val selectId = currentState.selectId ?: run {
+ sendEffect(VoteSideEffect.VoteFail)
+ return
+ }
+ viewModelScope.launch(Dispatchers.IO) {
+ setState { it.copy(isLoading = true, buttonEnabled = false) }
+ voteRepository.fetchCreateVotingItem(
+ votingTopicId = currentState.vote.id,
+ selectedId = selectId,
+ ).onSuccess {
+ setState { it.copy(buttonEnabled = false, isLoading = false) }
+ sendEffect(VoteSideEffect.VoteSuccess)
+ }.onFailure { error ->
+ setState { it.copy(isLoading = false, buttonEnabled = true) }
+ when (error) {
+ is ConflictException -> sendEffect(VoteSideEffect.VoteConflict)
+ else -> sendEffect(VoteSideEffect.VoteFail)
}
}
}
}π€ Prompt for AI Agents
In
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt
around lines 92 to 112, the postVote function has three issues: remove the
unused with(uiState.value) wrapper, stop using uiState.value.copy() inside
setState (use the lambda's it.copy() to avoid race conditions), and do not
fallback to UUID.randomUUID() when selectId is null β instead check selectId
before launching the vote, return early (and emit a suitable side effect or
setState to re-enable the button) so no vote is submitted with an invalid
selection. Ensure setState calls use the provided state lambda (it) and that the
coroutine is only started when a non-null selection exists.
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)
network/src/dev/kotlin/team.aliens.dms.android.network/voting/apiservice/VotingApiService.kt (1)
28-33: Retrofit does not natively supportResult<T>return types.Retrofit API methods must return
Call<T>,Response<T>, or response objects directly. The dev flavor'sResult<Unit>?return type will fail at runtime when Retrofit attempts to create the proxy implementation, as no customCallAdapter.FactoryforResulttypes is registered in the codebase.The prod flavor correctly returns
Response<Unit>?. Align the dev flavor with this approach and wrap the result inResultat the data source layer usingrunCatching:@POST("/votes/student/{voting-topic-id}") @RequiresAccessToken suspend fun fetchCreateVotingItem( @Path("voting-topic-id") votingTopicId: UUID, @Query("selected-id") selectedId: UUID, -): Result<Unit>? +): Response<Unit>?
β»οΈ Duplicate comments (1)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (1)
68-68:UUID.fromStringcan throwIllegalArgumentExceptionon invalid input.This issue was previously flagged. If
selectedIdis not a valid UUID string, this line will crash. Add validation or wrap in error handling as suggested in the earlier review.π Proposed fix
- onSelectItem = { selectedId -> viewModel.setSelectId(UUID.fromString(selectedId)) }, + onSelectItem = { selectedId -> + runCatching { UUID.fromString(selectedId) } + .onSuccess { viewModel.setSelectId(it) } + },
π§Ή Nitpick comments (1)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
369-369: Consider adding a default value for consistency.The
isLoadingparameter lacks a default value, requiring callers to always specify it. For consistency withDmsButton(which defaults tofalse), consider adding a default.π Proposed change
- isLoading: Boolean, + isLoading: Boolean = false,
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (8)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.ktdata/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepositoryImpl.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.ktnetwork/src/dev/kotlin/team.aliens.dms.android.network/voting/apiservice/VotingApiService.ktnetwork/src/dev/kotlin/team.aliens.dms.android.network/voting/datasource/NetworkVotingDataSource.ktnetwork/src/dev/kotlin/team.aliens.dms.android.network/voting/datasource/NetworkVotingDataSourceImpl.kt
π§ Files skipped from review as they are similar to previous changes (3)
- feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/ui/component/VoteContent.kt
- feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.kt
- data/src/dev/kotlin/team.aliens.dms.android.data/voting/repository/VotingRepositoryImpl.kt
π§° Additional context used
𧬠Code graph analysis (2)
network/src/dev/kotlin/team.aliens.dms.android.network/voting/datasource/NetworkVotingDataSourceImpl.kt (1)
core/network/src/main/java/team/aliens/dms/android/core/network/util/NetworkRequestHandler.kt (1)
handleNetworkRequest(15-50)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (2)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/component/VoteItemContent.kt (1)
VoteItemContent(11-71)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
DmsLayeredButton(359-391)
β° 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). (1)
- GitHub Check: build
π Additional comments (4)
feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt (3)
27-36: LGTM! Clean ViewModel integration.The function signature and ViewModel setup follow standard Compose and Hilt patterns correctly. State collection with lifecycle awareness is properly implemented.
73-88: Clean UI structure.The VoteScreen layout is well-organized with proper background styling and top app bar integration. The function signature clearly defines all dependencies.
103-118: Excellent button implementation with proper state management.The
DmsLayeredButtonis well-configured with:
- Loading state to prevent duplicate submissions
- Enabled state for validation feedback
- Appropriate shape for bottom-anchored positioning
- Clear call-to-action text
The state-driven approach ensures good UX during vote submission.
network/src/dev/kotlin/team.aliens.dms.android.network/voting/datasource/NetworkVotingDataSource.kt (1)
15-18: Verify consistency after fixing the API service layer.The non-nullable
Result<Unit>return type is appropriate at the data source interface level. However, the API service currently declaresResult<Unit>?(nullable), creating a type mismatch.Once the API service is corrected to return
Response<Unit>?(per the previous comment), ensure this interface and its implementation properly handle the wrapping.
| buttonType: ButtonType, | ||
| buttonColor: ButtonColor, | ||
| enabled: Boolean = true, | ||
| shape: RoundedCornerShape, |
There was a problem hiding this comment.
Shape parameter not forwarded to inner button despite comment intent.
The Korean comment on line 386-387 indicates the intention to pass a shape to the button, but the DmsButton call doesn't include a shape parameter. Additionally, the parameter is restricted to RoundedCornerShape while DmsButton accepts any Shape, making this inconsistently restrictive.
π Proposed fix to forward shape parameter
If the intent is to pass the shape to the inner button:
DmsButton(
modifier = Modifier
.fillMaxWidth()
.align(Alignment.Center),
text = text,
buttonType = buttonType,
buttonColor = buttonColor,
enabled = enabled,
- // 3. λ²νΌμλ λ³λμ shapeλ₯Ό μ λ¬ν©λλ€.
+ shape = shape,
isLoading = isLoading,
onClick = onClick,
)Alternatively, if the outer shape should remain independent, consider removing the misleading comment and updating the parameter type to accept generic Shape:
- shape: RoundedCornerShape,
+ shape: Shape,Also applies to: 378-389
π€ Prompt for AI Agents
In
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt
around line 366 (and similarly 378β389), the shape parameter is declared as
RoundedCornerShape but is not forwarded to the inner DmsButton call and the type
is unnecessarily restrictive; update the outer function to accept a generic
Shape (androidx.compose.ui.graphics.Shape) and pass that shape through to the
inner DmsButton shape parameter, or if outer shape should not be forwarded,
remove or update the misleading comment and change the parameter type to Shape
(or remove it entirely) so the signature and behavior are consistent.
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt
Outdated
Show resolved
Hide resolved
| title: String, | ||
| startTime: String, | ||
| endTime: String, |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
# First, let's locate and examine the VoteScreen.kt file
find . -name "VoteScreen.kt" -type fRepository: team-aliens/DMS-Android
Length of output: 145
π Script executed:
# Once we find it, let's examine the relevant lines
cat -n feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt | head -100Repository: team-aliens/DMS-Android
Length of output: 4326
π Script executed:
# Search for VoteViewModel to understand the initState implementation
find . -name "*VoteViewModel*" -type fRepository: team-aliens/DMS-Android
Length of output: 155
π Script executed:
# Look for date parsing related code in the vote feature
rg -n "LocalDateTime|parse|initState" feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/Repository: team-aliens/DMS-Android
Length of output: 2338
π Script executed:
# Find the toLocalDateTime extension function
find . -path "*/shared/date*" -name "*.kt" -type fRepository: team-aliens/DMS-Android
Length of output: 651
π Script executed:
# Search for the toLocalDateTime implementation
rg -n "fun.*toLocalDateTime" --type=ktRepository: team-aliens/DMS-Android
Length of output: 93
π Script executed:
# Get the full VoteViewModel implementation
cat -n ./feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/viewmodel/VoteViewModel.ktRepository: team-aliens/DMS-Android
Length of output: 5634
π Script executed:
# Get the Extensions.kt file which likely contains toLocalDateTime
cat -n ./shared/date/src/main/java/team/aliens/dms/android/shared/date/Extensions.ktRepository: team-aliens/DMS-Android
Length of output: 1614
π Script executed:
# Also check the test file to understand expected behavior
cat -n ./shared/date/src/test/java/team/aliens/dms/android/shared/date/junit/ExtensionsTest.ktRepository: team-aliens/DMS-Android
Length of output: 3176
Add error handling for date string parsing in initState.
The String.toLocalDateTime() extension (line 19-20 in shared/date/Extensions.kt) calls LocalDateTime.parse() without catching DateTimeParseException. The initState function passes startTime and endTime strings directly to this function with no error handling. Invalid date formats will crash the application. Wrap the parsing in a try-catch block or use a safe parsing utility that returns a default value or null on parsing failure.
π€ Prompt for AI Agents
In feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/ui/VoteScreen.kt
around lines 29-31, the startTime and endTime strings are passed directly to
String.toLocalDateTime() which can throw DateTimeParseException; wrap those
parses in a try-catch (catching DateTimeParseException) or call a safe parsing
helper that returns null/default on failure, assign the parsed values to
nullable state (or a sensible default LocalDateTime) and update downstream
code/UI to handle null/defaults so the app does not crash on invalid date
formats.
| override suspend fun fetchCreateVotingItem(votingTopicId: UUID, selectedId: UUID): Result<Unit> = runCatching { | ||
| handleNetworkRequest { votingApiService.fetchCreateVotingItem(votingTopicId, selectedId) } | ||
| } |
There was a problem hiding this comment.
Logic error: double-wrapping and bypassed error handling.
The implementation has two critical issues:
-
Double-wrapping:
votingApiService.fetchCreateVotingItemreturnsResult<Unit>?, which is then passed tohandleNetworkRequest(returningResult<Unit>?), and wrapped again withrunCatching. This creates nestedResulttypes rather than a singleResult<Unit>. -
Bypassed error handling:
handleNetworkRequestis designed to catchHttpExceptionand map status codes to domain exceptions. If the API service returnsResult<Unit>?, no exceptions are thrown, sohandleNetworkRequest's error handling logic is completely bypassed.
π Correct implementation
After fixing the API service to return Response<Unit>?:
-override suspend fun fetchCreateVotingItem(votingTopicId: UUID, selectedId: UUID): Result<Unit> = runCatching {
- handleNetworkRequest { votingApiService.fetchCreateVotingItem(votingTopicId, selectedId) }
-}
+override suspend fun fetchCreateVotingItem(votingTopicId: UUID, selectedId: UUID): Result<Unit> =
+ runCatching {
+ handleNetworkRequest { votingApiService.fetchCreateVotingItem(votingTopicId, selectedId) }
+ }This ensures:
- API service returns
Response<Unit>?(standard Retrofit) handleNetworkRequestcatches and maps HTTP errors to exceptionsrunCatchingwraps the entire operation inResult<Unit>
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
β»οΈ Duplicate comments (1)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
364-364: Shape parameter not forwarded to inner button.The
shapeparameter is accepted but not passed to the innerDmsButtoncall. This is the same issue flagged in the previous review. Additionally, the parameter type is restricted toRoundedCornerShapewhileDmsButtonaccepts anyShape, making this unnecessarily restrictive.π Proposed fix
fun DmsLayeredButton( modifier: Modifier = Modifier, text: String, buttonType: ButtonType, buttonColor: ButtonColor, enabled: Boolean = true, - shape: RoundedCornerShape, + shape: Shape = RoundedCornerShape(16.dp), backgroundColor: Color = DmsTheme.colorScheme.surfaceVariant, layerOffset: Dp = 24.dp, isLoading: Boolean, onClick: () -> Unit, ) { Box( modifier = modifier .background(color = backgroundColor, shape = shape) .windowInsetsPadding(WindowInsets.navigationBars) .padding(layerOffset), ) { DmsButton( modifier = Modifier .fillMaxWidth() .align(Alignment.Center), text = text, buttonType = buttonType, buttonColor = buttonColor, enabled = enabled, + shape = shape, isLoading = isLoading, onClick = onClick, ) } }Also applies to: 376-386
π§Ή Nitpick comments (5)
core/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.kt (1)
115-124: Good fix for local token expiry, but consider adding logging for other failures.The addition of
CannotUseRefreshTokenExceptionhandling correctly addresses the inconsistency where remote 401 cleared caches but local expiry didn't. However, the emptyelseblock still silently ignores network errors, 5xx responses, and other reissue failures, making debugging difficult.π Suggested improvement: Add logging to the else branch
exception is CannotUseRefreshTokenException -> { [email protected]() } - else -> {} + else -> { + // Log unhandled reissue failures for debugging + timber.log.Timber.w(exception, "Token reissue failed") + } }app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (2)
166-176: Simplify back navigation by using the captured nav key directly.Creating a new
VoteScreenNavinstance for removal is unnecessary sincevoteNavis already available and data class equality ensures they match.π Proposed fix
entry<VoteScreenNav> { voteNav -> VoteRoute( title = voteNav.title, startTime = voteNav.startTime, endTime = voteNav.endTime, - onNavigateBack = { backStack.remove(VoteScreenNav(voteNav.title, voteNav.startTime, voteNav.endTime)) }, + onNavigateBack = { backStack.remove(voteNav) }, onShowSnackBar = { snackBarType, message -> appState.showSnackBar(snackBarType, message) } ) }
177-187: Extract result key to a constant.The magic string
"remain_application_result"should be extracted to a named constant to prevent typos and ensure consistency when retrieving this result elsewhere.π Proposed fix
Define a constant at file or companion object level:
private const val REMAIN_APPLICATION_RESULT_KEY = "remain_application_result"Then update the usage:
entry<RemainScreenNav> { RemainApplicationRoute( onNavigateBack = { title -> - resultStore.setResult<String?>("remain_application_result", title) + resultStore.setResult<String?>(REMAIN_APPLICATION_RESULT_KEY, title) backStack.remove(RemainScreenNav) }, onShowSnackBar = { snackBarType, message -> appState.showSnackBar(snackBarType, message) } ) }app/build.gradle.kts (1)
143-143: Remove the commented-out desugaring dependency.With minSdk 26, core library desugaring is generally not required since Java 8+ APIs (such as
java.time) are natively available. The commented-out line should either be removed entirely or, if there's a specific reason to keep it, documented with a clear explanation.π Proposed fix
-// coreLibraryDesugaring(libs.desugar.jdk.libs)feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (1)
80-80: Extract hardcoded Korean text to string resource.The time window notice text is hardcoded directly in the composable, which makes internationalization and maintenance more difficult. Consider extracting it to a string resource with format arguments.
π Example approach
In
strings.xml:<string name="remain_application_time_notice">μλ₯ μ μ² μκ°μ %1$s %2$s ~ %3$s %4$s κΉμ§ μ λλ€.</string>Then in the composable:
text = stringResource( R.string.remain_application_time_notice, state.remainsApplicationTime.startDayOfWeek.toLocale(), state.remainsApplicationTime.startTime, state.remainsApplicationTime.endDayOfWeek.toLocale(), state.remainsApplicationTime.endTime )
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (9)
app/build.gradle.ktsapp/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.ktbuildSrc/src/main/kotlin/ProjectProperties.ktcore/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.ktcore/jwt/src/main/java/team/aliens/dms/android/core/jwt/JwtProviderImpl.ktdata/src/dev/kotlin/team.aliens.dms.android.data/remain/model/RemainsOption.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.ktfeature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.ktgradle/libs.versions.toml
π§° Additional context used
𧬠Code graph analysis (3)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (5)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/navigation/ResultStore.kt (1)
rememberResultStore(39-44)feature/src/dev/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationRoute.kt (1)
ApplicationRoute(9-24)feature/src/dev/kotlin/team/aliens/dms/android/feature/vote/navigation/VoteRoute.kt (1)
VoteRoute(8-23)feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/navigation/RemainApplicationRoute.kt (1)
RemainApplicationRoute(7-16)feature/src/dev/kotlin/team/aliens/dms/android/feature/meal/navigation/MealRoute.kt (1)
MealRoute(6-13)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (4)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/ui/component/DmsFloatingNotice.kt (1)
DmsFloatingNotice(29-65)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.kt (1)
DmsApplicationCard(33-116)feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (1)
setSelectRemainsOption(52-54)core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
DmsButton(260-356)
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (1)
core/ui/src/dev/java/team/aliens/dms/android/core/ui/viewmodel/BaseStateViewModel.kt (1)
setState(29-31)
β° 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). (1)
- GitHub Check: build
π Additional comments (8)
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt (1)
7-15: LGTM! Imports support the new layered button functionality.The new imports are appropriate for implementing
DmsLayeredButton. Note thatoffsetandsystemBarsPaddingare imported but not used in the visible code; however, they may be needed for future enhancements or other parts of the file not shown.Also applies to: 30-30
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (5)
11-35: LGTM!The new imports for
CompositionLocalProvider,LocalResultStore,rememberResultStore, and the new route components are all properly utilized in the file. The previously flagged unusedandroid.util.Logimport has been removed.
52-56: LGTM!The new navigation keys follow the established pattern in the file. Using
StringforstartTimeandendTimesimplifies serialization for navigation state persistence.
72-72: LGTM!Proper initialization of the result store using
rememberResultStore(), which is correctly provided viaCompositionLocalProviderat line 113.
151-165: Verify time string format is suitable for display.The
startTime.toString()andendTime.toString()conversions will use the defaultLocalDateTime(or similar) string representation. Ensure theVoteRouteeither parses these back or that the default format is acceptable for UI display.
191-195: LGTM!Using
backStack.remove(MealScreenNav)is appropriate sinceMealScreenNavis a data object (singleton), ensuring correct equality comparison.gradle/libs.versions.toml (1)
42-42: LGTM!The version catalog change is consistent with the MIN_SDK update in ProjectProperties.kt, maintaining proper synchronization across build configuration files.
feature/src/dev/kotlin/team/aliens/dms/android/feature/remain/viewmodel/RemainApplicationViewModel.kt (1)
64-81: Good improvement in null handling.The use of
?.letproperly prevents the function from executing whenselectRemainsOptionIdis null, addressing the previous critical issue where a random UUID would be sent to the server. Since the UI button is disabled when no option is selected (line 117 in RemainApplicationScreen.kt), this defensive approach is appropriate.
| object ProjectProperties { | ||
| const val COMPILE_SDK = 36 | ||
| const val MIN_SDK = 24 | ||
| const val MIN_SDK = 26 |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
#!/bin/bash
# Description: Search for references to old minSdk values that might need updating
# Search for hardcoded references to API 24 or 25
rg -nP '\b(24|25)\b' --type=xml -g '**/AndroidManifest.xml' -C2
# Search for minSdkVersion references in XML files
rg -nP 'minSdkVersion.*["\']?(24|25)["\']?' --type=xml -C2
# Search for SDK_INT comparisons that might need adjustment
rg -nP 'SDK_INT\s*[<>=]+\s*(24|25|Build\.VERSION_CODES\.(N|N_MR1))' --type=kotlin --type=java -C3Repository: team-aliens/DMS-Android
Length of output: 212
π Script executed:
#!/bin/bash
# Description: Search for references to old minSdk values that might need updating
echo "=== Searching for minSdkVersion in XML files ==="
rg 'minSdkVersion' --type=xml -C2
echo ""
echo "=== Searching for hardcoded references to API 24 or 25 ==="
rg '\b(24|25)\b' --type=xml --type=kotlin --type=java -C1
echo ""
echo "=== Searching for SDK_INT comparisons with N or N_MR1 ==="
rg 'SDK_INT|Build\.VERSION_CODES\.(N|N_MR1)' --type=kotlin --type=java -C2
echo ""
echo "=== Checking current MIN_SDK definition ==="
rg 'MIN_SDK' --type=kotlinRepository: team-aliens/DMS-Android
Length of output: 50380
π Script executed:
#!/bin/bash
# More targeted search for minSdk configuration and version checks
echo "=== Searching for build.gradle files with minSdk ==="
fd -e gradle -e kts | head -20 | xargs grep -l minSdk 2>/dev/null || echo "No matches"
echo ""
echo "=== Checking actual minSdk values in gradle files ==="
fd -e gradle -e kts | xargs grep -n "minSdk" 2>/dev/null | head -20
echo ""
echo "=== Searching for SDK_INT version checks in code ==="
rg 'SDK_INT' --type=kotlin --type=java -A2 | head -40
echo ""
echo "=== Searching for Build.VERSION_CODES references ==="
rg 'Build\.VERSION_CODES\.(N|NOUGAT)' --type=kotlin --type=java | head -20
echo ""
echo "=== Verify ProjectProperties.kt content ==="
cat buildSrc/src/main/kotlin/ProjectProperties.ktRepository: team-aliens/DMS-Android
Length of output: 5444
π Script executed:
#!/bin/bash
# Check the TODO and look for any version checks that are now unnecessary
echo "=== MealWorker.kt context around TODO ==="
cat -n core/widget/src/prod/kotlin/team/aliens/dms/android/core/widget/MealWorker.kt | grep -A 10 -B 5 "TODO"
echo ""
echo "=== Searching for version checks <= 26 that are now always true or false ==="
rg 'SDK_INT\s*[<].*\s*(26|25|24|O|N)' --type=kotlin --type=java
echo ""
echo "=== Check if there are conditions that assume API < 26 ==="
rg 'SDK_INT\s*<\s*(26|Build\.VERSION_CODES\.O)' --type=kotlin --type=javaRepository: team-aliens/DMS-Android
Length of output: 1333
Remove unreachable code in MealWorker.kt related to the minSdk increase.
The change from minSdk 24 to 26 is complete with no residual references to APIs 24-25 found in the codebase. However, the else branch in MealWorker.kt (line 42) containing TODO("VERSION.SDK_INT < O") is now unreachable since Build.VERSION_CODES.O equals API level 26. Remove this dead code path and keep only the PeriodicWorkRequestBuilder branch.
π€ Prompt for AI Agents
In buildSrc/src/main/kotlin/ProjectProperties.kt (minSdk changed to 26) and in
app/src/.../MealWorker.kt around line 42, remove the unreachable else branch
that contains TODO("VERSION.SDK_INT < O") and any dead references to
VERSION.SDK_INT checks; keep only the PeriodicWorkRequestBuilder branch (the
code path for API >= O), delete the else block and its TODO, and remove any
now-unused imports or variables introduced solely for that dead branch.
| buttonColor: ButtonColor, | ||
| enabled: Boolean = true, | ||
| shape: RoundedCornerShape, | ||
| backgroundColor: Color = DmsTheme.colorScheme.surfaceVariant, |
There was a problem hiding this comment.
Critical: backgroundColor parameter is unused; hard-coded white breaks theming.
Two related issues:
- The
backgroundColorparameter (line 365) is declared with a theme-aware default but never used in the function body. - Line 372 hard-codes
Color.Whitefor the background, breaking theming and dark mode support.
This suggests an incomplete implementation where the developer likely intended to use the backgroundColor parameter instead of the hard-coded value.
π Proposed fix
Box(
modifier = modifier
- .background(color = Color.White, shape = shape)
+ .background(color = backgroundColor, shape = shape)
.windowInsetsPadding(WindowInsets.navigationBars)
.padding(layerOffset),
) {Also applies to: 372-372
π€ Prompt for AI Agents
In
core/design-system/src/dev/java/team/aliens/dms/android/core/designsystem/button/Button.kt
around lines 365 and 372, the backgroundColor parameter declared at line 365 is
never used and the composable hard-codes Color.White at line 372, breaking
theming and dark mode; fix this by replacing the hard-coded Color.White with the
backgroundColor parameter (or a resolved theme-aware color) so the
passed-in/default DmsTheme.colorScheme.surfaceVariant is actually applied, and
remove any unused imports/values if they become redundant.
κ°μ
μμ μ¬ν
μΆκ° λ‘ ν λ§
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
βοΈ Tip: You can customize this high-level summary in your review settings.