๐ :: (#960) ์๋ฒฝ ์์ต ์ ์ฒญ ๊ธฐ๋ฅ ๊ตฌํ#926
Open
๐ :: (#960) ์๋ฒฝ ์์ต ์ ์ฒญ ๊ธฐ๋ฅ ๊ตฌํ#926
Conversation
|
No actionable comments were generated in the recent review. ๐ โน๏ธ Recent review infoโ๏ธ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ๐ Files selected for processing (1)
โ Files skipped from review due to trivial changes (1)
๐ WalkthroughWalkthroughAdds a complete "Late Study" feature: API, network datasource, repository + DI, domain models and mappers, ViewModel, Compose UI (screen + components + calendar), navigation key/route wiring, application-screen integration, and a design-system chip style. ChangesLate Study Feature
Sequence DiagramsequenceDiagram
actor User
participant UI as LateStudyScreen
participant VM as LateStudyViewModel
participant Repo as LateStudyRepository
participant ND as NetworkLateStudyDataSource
participant API as LateStudyApiService
participant AppVM as ApplicationViewModel
participant Store as LocalResultStore
User->>UI: Open Late Study Screen
UI->>VM: init -> fetchStudyTypes, fetchTeachers
VM->>Repo: fetchStudyTypes()
Repo->>ND: fetchStudyTypes()
ND->>API: GET /study-types
API-->>ND: FetchStudyTypesResponse
ND-->>Repo: Response
Repo-->>VM: List<StudyType>
VM->>Repo: fetchTeachers()
Repo->>ND: fetchTeachers()
ND->>API: GET /teachers
API-->>ND: FetchTeachersResponse
ND-->>Repo: Response
Repo-->>VM: List<TeacherResponse>
User->>UI: Select teacher/type/dates/reason
User->>UI: Tap Submit
UI->>VM: submitLateStudy(...)
VM->>Repo: submitLateStudy(SubmitLateStudyRequest)
Repo->>ND: submitLateStudy(request)
ND->>API: POST /submit
API-->>ND: Success
ND-->>Repo: Return
Repo-->>VM: Return
UI->>Store: set("late_study_application_result","์ ์ฒญ ์ค")
UI->>AppVM: onNavigateLateStudyApplication callback
AppVM->>Repo: fetchMyStudyApplicationStatus()
Repo->>ND: fetchMyStudyApplicationStatus()
ND->>API: GET /application-status
API-->>ND: StudyApplicationStatusResponse
ND-->>Repo: Response
Repo-->>AppVM: Response
AppVM->>AppVM: derive UI status & persist title
Estimated code review effort๐ฏ 4 (Complex) | โฑ๏ธ ~60 minutes Possibly related PRs
Poem
๐ฅ Pre-merge checks | โ 4 | โ 1โ Failed checks (1 warning)
โ Passed checks (4 passed)
โ๏ธ Tip: You can configure your own custom pre-merge checks in the settings. โจ Finishing Touches๐ Generate docstrings
๐งช Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
| } | ||
| if (title == null) { | ||
| sharedPreferences.edit().remove(KEY_APPLIED_LATE_STUDY_TITLE).apply() |
| if (title == null) { | ||
| sharedPreferences.edit().remove(KEY_APPLIED_LATE_STUDY_TITLE).apply() | ||
| } else { | ||
| sharedPreferences.edit().putString(KEY_APPLIED_LATE_STUDY_TITLE, title).apply() |
| } | ||
|
|
||
| internal fun setLateStudyApplication(title: String) { | ||
| sharedPreferences.edit().putString(KEY_APPLIED_LATE_STUDY_TITLE, title).apply() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
๐งน Nitpick comments (3)
feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyReasonSection.kt (1)
44-45: โก Quick winUse
REASON_MAX_LENGTHconsistently instead of hardcoded200.Line 44 and Line 65 duplicate the max-length literal even though a constant already exists. This can drift later.
Proposed change
- Text( - text = "${value.length}/200", + Text( + text = "${value.length}/$REASON_MAX_LENGTH", color = DmsTheme.colorScheme.inverseSurface, style = DmsTheme.typography.labelM, modifier = Modifier.padding(top = 4.dp, end = 10.dp) ) @@ - onValueChange = { newValue -> - if (newValue.length <= 200) { + onValueChange = { newValue -> + if (newValue.length <= REASON_MAX_LENGTH) { onValueChange(newValue) } },Also applies to: 65-66
๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyReasonSection.kt` around lines 44 - 45, Replace the hardcoded max-length literal 200 with the existing REASON_MAX_LENGTH constant in the LateStudyReasonSection composable: update the text display that currently uses "${value.length}/200" and any other occurrences (e.g., maxLength parameters or checks around value in LateStudyReasonSection) to reference REASON_MAX_LENGTH instead (ensure the constant is in scope or import it if needed) so both places use the single source of truth.feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt (1)
37-59: โก Quick winDecouple business mapping from display strings.
Behavior is keyed off localized title text (
when (title)), which is brittle. A text/resource change can breakappliedTitleandchipStylemapping without compile-time signal.Use a stable card type (enum/sealed class) and map title/chip/appliedTitle off that type.
๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt` around lines 37 - 59, The UI currently switches behavior using localized title strings (the Triple list and the when(title) blocks in ApplicationContent.kt) which is brittle; replace that by introducing a stable card identifier (e.g., an enum or sealed class like ApplicationCardType) and build a list of card models (data class with type: ApplicationCardType, titleRes/iconRes/onClick) instead of Triples; then update DmsApplicationCard usages to derive appliedTitle and chipStyle by switching on the card.type (use ApplicationCardType.REMAIN, LATE_STUDY, OUTING, VOLUNTEER) rather than the title string so UI text can change without breaking logic (adjust any references to appliedTitle and chipStyle to use the new type-based when branches).data/src/main/kotlin/team/aliens/dms/android/data/latestudy/repository/LateStudyRepository.kt (1)
4-16: ๐๏ธ Heavy liftAvoid exposing network DTOs in repository contracts.
LateStudyRepositoryis a data-layer boundary, but it currently imports and exposes network models directly. This couples feature/data code to transport schema changes and makes future API changes ripple farther than needed.Please introduce repository-facing models/commands (or domain models) and map in
LateStudyRepositoryImpl.๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/kotlin/team/aliens/dms/android/data/latestudy/repository/LateStudyRepository.kt` around lines 4 - 16, LateStudyRepository currently exposes network DTOs (StudyApplicationStatusResponse, TeacherResponse, SubmitLateStudyRequest) in its public API; change the interface to use repository/domain models (e.g., StudyType (if already domain), Teacher, StudyApplicationStatus, SubmitLateStudyCommand) instead of the network types for the methods fetchTeachers(), fetchMyStudyApplicationStatus(), and submitLateStudy(); then implement mapping in LateStudyRepositoryImpl to convert network DTOs to these domain models on fetch and convert the repository command/model to SubmitLateStudyRequest when sending to the network. Ensure all references to StudyApplicationStatusResponse, TeacherResponse, and SubmitLateStudyRequest are removed from the interface and only appear inside LateStudyRepositoryImpl mapping code.
๐ค Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyCalendarSection.kt`:
- Around line 68-73: The Icon usages in LateStudyCalendarSection (the left and
right month controls) use Modifier.clickable which yields a 24ร24 dp tap area;
replace those Icons with IconButton wrappers (one for the previous-month control
tied to onPrevMonthClick and one for the next-month control tied to
onNextMonthClick) so the touch target is at least 48ร48 dp and the control is
announced as a button; preserve the existing contentDescription and tint, move
any modifier (padding/size) to the IconButton as appropriate, and remove the
.clickable from the inner Icon.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyTypeItem.kt`:
- Around line 34-45: The Row currently uses .clickable(onClick = onClick) which
doesn't expose role/selected state to accessibility services; update the
modifier chain in LateStudyTypeItem's Row to use .selectable(selected =
selected, onClick = onClick, role = Role.RadioButton) instead of .clickable so
assistive tech can announce selection, and add the necessary import for
androidx.compose.foundation.selection.selectable and
androidx.compose.ui.semantics.Role if missing.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/LateStudyScreen.kt`:
- Around line 302-320: The click handler currently re-checks endDate and rejects
one-day submissions; update the onClick validation to match the button enabled
logic by removing the endDate != null condition so it only requires
selectedTeacherId, selectedTypeId, startDate, and non-blank reason before
calling viewModel.submitLateStudy; keep the existing fallback endDate ?:
startDate usage and pass teacherId = selectedTeacherId!!, typeId =
selectedTypeId!!, reason, startDate.toString(), and (endDate ?:
startDate).toString() as before.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.kt`:
- Around line 59-80: The submitLateStudy function currently fire-and-forgets and
swallows exceptions; change its API so callers can observe success/failure: make
TeamAliensDMS.android.feature.latestudy.viewmodel.LateStudyViewModel.submitLateStudy
a suspend function (or return a Result<Boolean>/Deferred/Flow) that directly
calls lateStudyRepository.submitLateStudy and returns success or throws the
repository exception instead of catching it; remove the internal
viewModelScope.launch and e.printStackTrace(), or alternatively keep the launch
but post the outcome to a LiveData/StateFlow (e.g., lateStudyApplicationResult)
so the screen can react to success/failure; ensure to reference submitLateStudy
and lateStudyRepository.submitLateStudy when updating callers.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt`:
- Around line 57-66: The current LaunchedEffect uses snapshotFlow over
resultStore.resultStateMap[...] which is a plain mutableMap and not
snapshot-aware, so snapshotFlow may never re-emit when keys are later added;
change the store to expose snapshot-aware state (e.g., make resultStateMap a
SnapshotStateMap or expose a State/MutableState for each result key) and then
collect from snapshotFlow (or directly observe the State) instead of reading a
plain Map; specifically update the resultStore/resultStateMap implementation so
setResult(...) inserts into a SnapshotStateMap (or exposes an observable
getResultState(key): State<String?>) and then in ApplicationScreen's
LaunchedEffect use that snapshot-aware State to call
viewModel.setLateStudyApplication(result) and resultStore.removeResult(resultKey
= "late_study_application_result").
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/viewmodel/ApplicationViewModel.kt`:
- Around line 78-88: The ViewModel preserves in-memory late-study fields when
the API returns null, causing a stale chip; update the setState call in
ApplicationViewModel (the lambda passed to setState) to assign the nullable
values directly (e.g., lateStudyApplicationTitle = title and lateStudyStatusUi =
statusUi) instead of falling back to current.* so that when title or statusUi
are null the in-memory state is cleared; keep the sharedPreferences
removal/putString logic with KEY_APPLIED_LATE_STUDY_TITLE as-is to remain
consistent with persisted state.
---
Nitpick comments:
In
`@data/src/main/kotlin/team/aliens/dms/android/data/latestudy/repository/LateStudyRepository.kt`:
- Around line 4-16: LateStudyRepository currently exposes network DTOs
(StudyApplicationStatusResponse, TeacherResponse, SubmitLateStudyRequest) in its
public API; change the interface to use repository/domain models (e.g.,
StudyType (if already domain), Teacher, StudyApplicationStatus,
SubmitLateStudyCommand) instead of the network types for the methods
fetchTeachers(), fetchMyStudyApplicationStatus(), and submitLateStudy(); then
implement mapping in LateStudyRepositoryImpl to convert network DTOs to these
domain models on fetch and convert the repository command/model to
SubmitLateStudyRequest when sending to the network. Ensure all references to
StudyApplicationStatusResponse, TeacherResponse, and SubmitLateStudyRequest are
removed from the interface and only appear inside LateStudyRepositoryImpl
mapping code.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyReasonSection.kt`:
- Around line 44-45: Replace the hardcoded max-length literal 200 with the
existing REASON_MAX_LENGTH constant in the LateStudyReasonSection composable:
update the text display that currently uses "${value.length}/200" and any other
occurrences (e.g., maxLength parameters or checks around value in
LateStudyReasonSection) to reference REASON_MAX_LENGTH instead (ensure the
constant is in scope or import it if needed) so both places use the single
source of truth.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.kt`:
- Around line 37-59: The UI currently switches behavior using localized title
strings (the Triple list and the when(title) blocks in ApplicationContent.kt)
which is brittle; replace that by introducing a stable card identifier (e.g., an
enum or sealed class like ApplicationCardType) and build a list of card models
(data class with type: ApplicationCardType, titleRes/iconRes/onClick) instead of
Triples; then update DmsApplicationCard usages to derive appliedTitle and
chipStyle by switching on the card.type (use ApplicationCardType.REMAIN,
LATE_STUDY, OUTING, VOLUNTEER) rather than the title string so UI text can
change without breaking logic (adjust any references to appliedTitle and
chipStyle to use the new type-based when branches).
๐ช Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
โน๏ธ Review info
โ๏ธ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b791a9af-5511-492b-8b89-435b4c17f2c6
โ Files ignored due to path filters (1)
core/design-system/src/main/res/drawable/img_3d_dawn.pngis excluded by!**/*.png
๐ Files selected for processing (29)
app/src/main/kotlin/team/aliens/dms/android/app/navigation/DmsNavKeys.ktapp/src/main/kotlin/team/aliens/dms/android/app/ui/DmsApp.ktcore/design-system/src/main/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.ktdata/src/main/kotlin/team/aliens/dms/android/data/latestudy/di/LateStudyRepositoryModule.ktdata/src/main/kotlin/team/aliens/dms/android/data/latestudy/mapper/StudyTypeMapper.ktdata/src/main/kotlin/team/aliens/dms/android/data/latestudy/model/StudyType.ktdata/src/main/kotlin/team/aliens/dms/android/data/latestudy/repository/LateStudyRepository.ktdata/src/main/kotlin/team/aliens/dms/android/data/latestudy/repository/LateStudyRepositoryImpl.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/navigation/LateStudyRoute.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/LateStudyScreen.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyCalendarSection.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyReasonSection.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudySectionCard.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyTeacherSection.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyTypeItem.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/main/application/navigation/ApplicationRoute.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/component/ApplicationContent.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/main/application/viewmodel/ApplicationViewModel.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/apiservice/LateStudyApiService.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/datasource/NetworkLateStudyDataSource.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/datasource/NetworkLateStudyDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/di/LateStudyApiServiceModule.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/di/LateStudyDataSourceModule.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/model/FetchStudyTypesResponse.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/model/StudyApplicationStatusResponse.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/model/SubmitLateStudyRequest.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/model/TeacherResponse.kt
Comment on lines
+59
to
+80
| fun submitLateStudy( | ||
| teacherId: String, | ||
| typeId: String, | ||
| reason: String, | ||
| startDate: String, | ||
| endDate: String, | ||
| ) { | ||
| viewModelScope.launch { | ||
| try { | ||
| lateStudyRepository.submitLateStudy( | ||
| SubmitLateStudyRequest( | ||
| teacher_id = teacherId, | ||
| type_id = typeId, | ||
| reason = reason, | ||
| start_date = startDate, | ||
| end_date = endDate, | ||
| ), | ||
| ) | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() | ||
| } | ||
| } |
There was a problem hiding this comment.
Return submission success to the caller instead of fire-and-forget.
submitLateStudy() launches internally and swallows failures, so the caller cannot know whether the request actually succeeded. The current screen flow updates late_study_application_result and pops immediately after invoking this method, which means a network failure still looks like a submitted application to the user.
๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.kt`
around lines 59 - 80, The submitLateStudy function currently fire-and-forgets
and swallows exceptions; change its API so callers can observe success/failure:
make
TeamAliensDMS.android.feature.latestudy.viewmodel.LateStudyViewModel.submitLateStudy
a suspend function (or return a Result<Boolean>/Deferred/Flow) that directly
calls lateStudyRepository.submitLateStudy and returns success or throws the
repository exception instead of catching it; remove the internal
viewModelScope.launch and e.printStackTrace(), or alternatively keep the launch
but post the outcome to a LiveData/StateFlow (e.g., lateStudyApplicationResult)
so the screen can react to success/failure; ensure to reference submitLateStudy
and lateStudyRepository.submitLateStudy when updating callers.
Comment on lines
+57
to
+66
| LaunchedEffect(Unit) { | ||
| snapshotFlow { | ||
| resultStore.resultStateMap["late_study_application_result"]?.value as? String? | ||
| }.collect { result -> | ||
| if (result != null) { | ||
| viewModel.setLateStudyApplication(result) | ||
| resultStore.removeResult<String?>(resultKey = "late_study_application_result") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Web query:
In Jetpack Compose, does snapshotFlow { map[key]?.value }re-emit whenmapis a plainmutableMapOf() and the key is inserted later, or does it only track snapshot-aware state reads?
๐ก Result:
No, snapshotFlow { map[key]?.value } does not re-emit when map is a plain mutableMapOf and the key is inserted later. It only tracks snapshot-aware state reads.
Citations:
- 1: https://composables.com/jetpack-compose/androidx.compose.runtime/runtime/functions/snapshotFlow/api
- 2: https://stackoverflow.com/questions/76400468/jetpack-compose-snapshotflow-not-working-as-expected
- 3: https://developer.android.com/develop/ui/compose/side-effects
- 4: https://stackoverflow.com/questions/70404434/manually-observe-snapshotstatelist
- 5: https://stackoverflow.com/questions/77350449/why-snapshotflow-is-trigged-with-mutablestate-as-property-delegate
๐ Script executed:
# Find and read the ResultStore implementation
fd -e kt -e java ResultStore | head -5Repository: team-aliens/DMS-Android
Length of output: 147
๐ Script executed:
cat -n core/ui/src/main/java/team/aliens/dms/android/core/ui/navigation/ResultStore.ktRepository: team-aliens/DMS-Android
Length of output: 2148
๐ Script executed:
# Now let's check the ApplicationScreen usage context
head -70 feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt | tail -20Repository: team-aliens/DMS-Android
Length of output: 779
๐ Script executed:
# Let's check if there are other similar patterns with resultStore in the codebase
rg "snapshotFlow" feature/src/main/kotlin/team/aliens/dms/android/feature --context 3 -A 5Repository: team-aliens/DMS-Android
Length of output: 5940
๐ Script executed:
# Check if late_study_application_result is ever pre-initialized anywhere
rg "late_study_application_result" feature/ --context 2Repository: team-aliens/DMS-Android
Length of output: 1941
Use a snapshot-aware container for the result store.
snapshotFlow only re-emits when the lambda reads snapshot state. resultStateMap is a plain mutableMapOf(), so if the key is missing on first composition, the flow never subscribes to that key's state. When the key is later inserted via setResult() (e.g., after the user returns from LateStudyScreen), snapshotFlow will not re-emit, and the UI update will be missed entirely.
This pattern affects multiple screens: ApplicationScreen (both remain and late-study results), VoteScreen, and NoticeDetailScreen. Consider wrapping result state in a snapshot-aware data structure like SnapshotStateMap or restructuring to observe the store itself rather than nested map accesses.
๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/ui/ApplicationScreen.kt`
around lines 57 - 66, The current LaunchedEffect uses snapshotFlow over
resultStore.resultStateMap[...] which is a plain mutableMap and not
snapshot-aware, so snapshotFlow may never re-emit when keys are later added;
change the store to expose snapshot-aware state (e.g., make resultStateMap a
SnapshotStateMap or expose a State/MutableState for each result key) and then
collect from snapshotFlow (or directly observe the State) instead of reading a
plain Map; specifically update the resultStore/resultStateMap implementation so
setResult(...) inserts into a SnapshotStateMap (or exposes an observable
getResultState(key): State<String?>) and then in ApplicationScreen's
LaunchedEffect use that snapshot-aware State to call
viewModel.setLateStudyApplication(result) and resultStore.removeResult(resultKey
= "late_study_application_result").
Comment on lines
+78
to
+88
| setState { current -> | ||
| current.copy( | ||
| lateStudyApplicationTitle = title ?: current.lateStudyApplicationTitle, | ||
| lateStudyStatusUi = statusUi ?: current.lateStudyStatusUi, | ||
| ) | ||
| } | ||
| if (title == null) { | ||
| sharedPreferences.edit().remove(KEY_APPLIED_LATE_STUDY_TITLE).apply() | ||
| } else { | ||
| sharedPreferences.edit().putString(KEY_APPLIED_LATE_STUDY_TITLE, title).apply() | ||
| } |
There was a problem hiding this comment.
Clear the in-memory late-study state when the API returns no active item.
Lines 80-81 preserve the previous title/status when toAppliedTitle() or toUiStatus() return null, but Lines 84-87 still delete the stored preference. That leaves a stale late-study chip on screen until this ViewModel is recreated.
Suggested fix
setState { current ->
current.copy(
- lateStudyApplicationTitle = title ?: current.lateStudyApplicationTitle,
- lateStudyStatusUi = statusUi ?: current.lateStudyStatusUi,
+ lateStudyApplicationTitle = title,
+ lateStudyStatusUi = statusUi,
)
}๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/main/application/viewmodel/ApplicationViewModel.kt`
around lines 78 - 88, The ViewModel preserves in-memory late-study fields when
the API returns null, causing a stale chip; update the setState call in
ApplicationViewModel (the lambda passed to setState) to assign the nullable
values directly (e.g., lateStudyApplicationTitle = title and lateStudyStatusUi =
statusUi) instead of falling back to current.* so that when title or statusUi
are null the in-memory state is cleared; keep the sharedPreferences
removal/putString logic with KEY_APPLIED_LATE_STUDY_TITLE as-is to remain
consistent with persisted state.
There was a problem hiding this comment.
Actionable comments posted: 2
๐งน Nitpick comments (1)
feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.kt (1)
25-26: โก Quick win
selectedTypeIdstate andselectStudyType()are dead code.
LateStudyScreenmaintains its own localselectedTypeIdviaremember { mutableStateOf(null) }and never callsviewModel.selectStudyType(). The ViewModel'sselectedTypeIdis alwaysnull. These can be removed;submitLateStudyalready receivestypeIdas a parameter.โป๏ธ Proposed fix
- var selectedTypeId by mutableStateOf<String?>(null) - private set - var teachers by mutableStateOf<List<TeacherResponse>>(emptyList()) private set - fun selectStudyType(typeId: String) { - selectedTypeId = typeId - } - fun submitLateStudy(Also applies to: 56-58
๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.kt` around lines 25 - 26, Remove the dead ViewModel state and method: delete the nullable mutableState property selectedTypeId and the selectStudyType() function from LateStudyViewModel since LateStudyScreen holds its own selectedTypeId and never invokes selectStudyType(); ensure submitLateStudy still accepts a typeId parameter and leave it unchanged (so callers pass the screen-local typeId directly). Also remove any references/usages of selectedTypeId within LateStudyViewModel (and any related private setter) to avoid unused declarations.
๐ค Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/LateStudyScreen.kt`:
- Around line 192-194: The IconButton's touch target was reduced by using
Modifier.size(24.dp); remove the Modifier.size(24.dp) from the IconButton (the
call where IconButton(onClick = onBack, modifier = Modifier.size(24.dp))) so it
keeps Material3's default 48 dp interactive area; if you intended the visible
icon to be 24 dp, move Modifier.size(24.dp) to the Icon composable inside the
IconButton instead.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.kt`:
- Around line 69-77: The SubmitLateStudyRequest is being constructed with
snake_case parameter names causing compilation errors; update the call in
LateStudyViewModel (where lateStudyRepository.submitLateStudy(...) is invoked)
to use the Kotlin constructor parameter names: teacherId, typeId, reason,
startDate and endDate (camelCase matching the SubmitLateStudyRequest primary
constructor) so the request compiles and Gson `@SerializedName` can still map to
JSON.
---
Nitpick comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.kt`:
- Around line 25-26: Remove the dead ViewModel state and method: delete the
nullable mutableState property selectedTypeId and the selectStudyType() function
from LateStudyViewModel since LateStudyScreen holds its own selectedTypeId and
never invokes selectStudyType(); ensure submitLateStudy still accepts a typeId
parameter and leave it unchanged (so callers pass the screen-local typeId
directly). Also remove any references/usages of selectedTypeId within
LateStudyViewModel (and any related private setter) to avoid unused
declarations.
๐ช Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
โน๏ธ Review info
โ๏ธ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b47b2d9-9e8d-4e95-962d-fe5261d89185
๐ Files selected for processing (8)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/card/ApplicationChipStyle.ktcore/design-system/src/main/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/LateStudyScreen.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyCalendarSection.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyReasonSection.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyTypeItem.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/viewmodel/LateStudyViewModel.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/latestudy/model/SubmitLateStudyRequest.kt
โ Files skipped from review due to trivial changes (1)
- network/src/main/kotlin/team/aliens/dms/android/network/latestudy/model/SubmitLateStudyRequest.kt
๐ง Files skipped from review as they are similar to previous changes (2)
- core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/card/DmsApplicationCard.kt
- feature/src/main/kotlin/team/aliens/dms/android/feature/latestudy/ui/component/LateStudyTypeItem.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
๊ฐ์
์์ ์ฌํญ
์ถ๊ฐ ๋ก ํ ๋ง
Summary by CodeRabbit
New Features
Style
Bug Fixes