π :: (#822) λ€ν¬ν λ§ μ μμ΄μ½ μ»¬λ¬ λλΉ ν΄κ²°#823
Conversation
uson1004
added
performance
|
Caution Review failedThe pull request is closed. π WalkthroughWalkthroughAdds launcher assets and manifest updates, introduces product flavors, implements device token DataStore with DI and exceptions, adds a "latest notice" networkβdataβUI flow, extracts notification item UI, renames plusβbonus point fields, adds account withdrawal flow, and miscellaneous UI/CI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant HomeVM as HomeViewModel
participant Repo as NoticeRepository
participant Network as NetworkNoticeDataSource
participant API as NoticeApiService
HomeVM->>Repo: fetchLatestNotice()
Repo->>Network: fetchLatestNotice()
Network->>API: GET /notices/latest (with access token)
API-->>Network: FetchLatestNoticeResponse (id, title)
Network-->>Repo: Result<FetchLatestNoticeResponse>
Repo-->>HomeVM: Result<LatestNotice> (mapped)
HomeVM->>HomeVM: update state.latestNotice / emit FailFetchLatestNotice on error
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
π₯ Pre-merge checks | β 1 | β 2β Failed checks (2 warnings)
β Passed checks (1 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing touches
π§ͺ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (2)
feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/ui/NotificationScreen.kt (1)
220-246: Same modifier reuse issue exists here.
NotificationItemhas the same bug as the extractedNoticeItem- the outermodifierparameter is incorrectly reused for inner composables (Column,Row,Icon,Text,Spacer), which would apply unintended modifiers likefillMaxWidth()andclickable()to child elements.π Proposed fix
Column( - modifier = modifier.startPadding(12.dp), + modifier = Modifier.startPadding(12.dp), ) { Text( text = notification.title, style = DmsTheme.typography.bodyM, ) Row( - modifier = modifier.topPadding(6.dp) + modifier = Modifier.topPadding(6.dp) ) { if (!notification.isRead) { Icon( - modifier = modifier.size(4.dp), + modifier = Modifier.size(4.dp), imageVector = Icons.Filled.Circle, contentDescription = null, tint = DmsTheme.colorScheme.primaryContainer, ) } Text( - modifier = modifier + modifier = Modifier .startPadding(4.dp), text = notification.content, style = DmsTheme.typography.labelM, ) } } - Spacer(modifier = modifier.weight(1f)) + Spacer(modifier = Modifier.weight(1f))feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/ui/SettingScreen.kt (1)
49-111: Prevent multiple withdraw submissions.The confirm button stays enabled and the dialog remains open, so users can trigger multiple destructive requests. Please disable the button and/or show loading until the request finishes (and reset on success/failure).
β Suggested fix (pairs with a WithdrawFailed side effect)
- val (shouldShowWithdrawDialog, onShouldShowWithdrawDialogChange) = remember { - mutableStateOf(false) - } + val (shouldShowWithdrawDialog, onShouldShowWithdrawDialogChange) = remember { mutableStateOf(false) } + val (isWithdrawing, onWithdrawingChange) = remember { mutableStateOf(false) } @@ when (it) { SettingSideEffect.CannotFetchNotificationStatus -> { onShowSnackBar(DmsSnackBarType.ERROR, "μλ¦Ό μν μ‘°νλ₯Ό μ€ν¨νμ΄μ") } SettingSideEffect.SignOutSuccess -> onNavigateSignIn() - SettingSideEffect.WithdrawSuccess -> onNavigateSignIn() + SettingSideEffect.WithdrawSuccess -> { + onWithdrawingChange(false) + onNavigateSignIn() + } + SettingSideEffect.WithdrawFailed -> { + onWithdrawingChange(false) + onShowSnackBar(DmsSnackBarType.ERROR, "νμ νν΄μ μ€ν¨νμ΄μ") + } } } } @@ confirmButton = { DmsButton( text = "νμΈ", buttonType = ButtonType.Text, buttonColor = ButtonColor.Primary, - onClick = viewModel::withdraw, + enabled = !isWithdrawing, + isLoading = isWithdrawing, + onClick = { + onWithdrawingChange(true) + viewModel.withdraw() + }, ) },
π€ Fix all issues with AI agents
In `@app/src/dev/AndroidManifest.xml`:
- Around line 14-24: In the production AndroidManifest change the insecure
attributes so they are disabled: set android:allowBackup="false" and
android:usesCleartextTraffic="false" in the <application> element (the manifest
that currently contains android:name=".android.app.DevApplication" /
tools:replace="android:name"); update the production manifest's <application>
attributes to explicitly use these false values to prevent backups and block
cleartext HTTP in production.
In
`@core/device/src/prod/kotlin/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSourceImpl.kt`:
- Line 4: Remove the unused import suspendRunCatching from the top of
DeviceDataStoreDataSourceImpl (it's not used in the prod implementation which
directly delegates calls); update the imports by deleting the line importing
team.aliens.dms.android.shared.exception.util.suspendRunCatching so there are no
unused imports remaining.
In
`@core/device/src/prod/kotlin/team/aliens/dms/android/core/device/datastore/store/DeviceStoreImpl.kt`:
- Line 7: The import kotlinx.coroutines.runBlocking in DeviceStoreImpl is
unused; remove the unused import line (the runBlocking import) from the top of
DeviceStoreImpl.kt to clean up imports and avoid a lint warning.
- Around line 33-37: clearDeviceToken currently clears all preferences; change
it to remove only the device token key by calling deviceDataStore.edit {
preferences.remove(DEVICE_TOKEN_KEY) } inside the existing transform call
(mirror the pattern used in storeDeviceToken). Also add consistent error
handling like storeDeviceToken by attaching onFailure to the transform result
(or otherwise handling exceptions) so failures are logged/propagated the same
way.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.kt`:
- Around line 94-98: The outer Column in HomeScreen (the composable using
Modifier.fillMaxSize().background(DmsTheme.colorScheme.background).statusBarsPadding())
is missing bottom inset handling; update its modifier chain to also call
navigationBarsPadding() so the bottom action buttons aren't overlapped by
gesture/navigation barsβi.e., locate the Column in HomeScreen.kt and append
.navigationBarsPadding() to the existing modifier chain (preserving
.statusBarsPadding()) to match MyPageScreen/ApplicationScreen.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/ui/component/NoticeItem.kt`:
- Around line 46-72: The inner composables are incorrectly reusing the
NoticeItem parameter 'modifier' (applied to Column, Row, Icon, Text, Spacer),
which can leak outer modifiers into children; update the file so only the root
composable receives the passed-in 'modifier' and replace all inner uses with
fresh Modifier instances (e.g., Column(... = modifier /* keep only at root */)
but Row(... = Modifier.topPadding(...)), Icon(... = Modifier.size(...)),
Text(... = Modifier.startPadding(...)), Spacer(Modifier.weight(1f)), etc.),
ensuring you reference the Column, Row, Icon, Text, Spacer and the 'modifier'
parameter in NoticeItem when making the change.
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt`:
- Around line 99-103: The current remember call that computes pointHistoryList
using keys (page, state.allPointList) can return stale data when
state.bonusPointList or state.minusPointList change; update the computation in
PointHistoryScreen by either removing the remember (compute pointHistoryList
directly) or expanding the remember keys to include state.bonusPointList and
state.minusPointList and tabData[page], so that pointHistoryList reacts to
changes; locate the remember invocation that assigns pointHistoryList and modify
it accordingly (symbols: pointHistoryList, remember, tabData,
PointTab.All/Bonus/Minus,
state.allPointList/state.bonusPointList/state.minusPointList).
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt`:
- Around line 48-54: The withdraw() coroutine currently ignores failures; update
the studentRepository.withdraw() call to handle onFailure and emit a failure
side effect so UI can show an error and reset loading stateβe.g., in the block
where you call studentRepository.withdraw() inside
viewModelScope.launch(Dispatchers.IO), add .onFailure {
sendEffect(SettingSideEffect.WithdrawFailure(it)) } (or include a message from
the Throwable) alongside the existing
sendEffect(SettingSideEffect.WithdrawSuccess), and make the same change for the
other similar withdraw call in this file.
π§Ή Nitpick comments (5)
app/src/dev/res/drawable/ic_launcher_background.xml (1)
1-170: Consider removing this unused drawable.This appears to be the default Android Studio template for adaptive icon backgrounds (green with grid pattern). However, the adaptive icon definitions in
ic_launcher.xmlandic_launcher_round.xmlreference@color/ic_launcher_background(the white color resource), not this drawable.If this file is intentionally kept as a fallback or for future use, consider adding a comment explaining its purpose. Otherwise, removing it would reduce confusion.
app/src/dev/AndroidManifest.xml (1)
26-29: Consider tablet/foldable accessibility.Locking
screenOrientation="portrait"prevents landscape usage on tablets and foldables, which may impact accessibility and user experience on larger form factors. If tablet support is planned, consider making this configurable or allowing landscape on larger screens using resource qualifiers.feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/model/AnnouncementButton.kt (1)
49-53: Constrain long titles to avoid row height blowβups.
Dynamic notice titles can be long; keep the pill height stable by limiting lines and ellipsizing.β»οΈ Suggested tweak
Text( modifier = Modifier.padding(start = 8.dp), text = title, style = DmsTheme.typography.labelM, color = DmsTheme.colorScheme.inverseSurface, + maxLines = 1, + overflow = TextOverflow.Ellipsis, )feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/viewmodel/HomeViewModel.kt (2)
16-19: Prefer private injected repositories.Avoid exposing repositories as public properties on the ViewModel API.
β»οΈ Proposed refactor
internal class HomeViewModel `@Inject` constructor( - val studentRepository: StudentRepository, - val noticeRepository: NoticeRepository + private val studentRepository: StudentRepository, + private val noticeRepository: NoticeRepository ) : BaseStateViewModel<HomeState, HomeSideEffect>(HomeState()) {
53-56: Represent "no notice" explicitly to distinguish loading/error states from valid data.The current approach uses default
LatestNotice(id=null, title="")for both initial state and on fetch failure. SincegetLatestNotice().onFailure()doesn't update state, the UI always shows an emptyAnnouncementButtonregardless of whether data failed to load or hasn't been fetched yet. This prevents proper error feedback to users and may allow navigation with a null notice ID.Making
latestNoticenullable allows the UI to conditionally render the announcement section and properly distinguish between missing data and real content.β Suggested change
internal data class HomeState( val myPage: MyPage = MyPage(), - val latestNotice: LatestNotice = LatestNotice(), + val latestNotice: LatestNotice? = null, )
| android:name=".android.app.DevApplication" | ||
| tools:replace="android:name" | ||
| android:allowBackup="true" | ||
| android:enableOnBackInvokedCallback="true" | ||
| android:icon="@mipmap/ic_launcher" | ||
| android:label="@string/app_name" | ||
| android:roundIcon="@mipmap/ic_launcher_round" | ||
| android:supportsRtl="true" | ||
| android:theme="@style/Theme.DMS" | ||
| android:usesCleartextTraffic="true" | ||
| tools:targetApi="tiramisu"> |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
#!/bin/bash
# Find all AndroidManifest.xml files
fd -t f 'AndroidManifest.xml'Repository: team-aliens/DMS-Android
Length of output: 946
π Script executed:
#!/bin/bash
# Check allowBackup and usesCleartextTraffic in app manifests
for manifest in "app/src/main/AndroidManifest.xml" "app/src/dev/AndroidManifest.xml" "app/src/prod/AndroidManifest.xml"; do
if [ -f "$manifest" ]; then
echo "=== $manifest ==="
cat -n "$manifest" | grep -E "allowBackup|usesCleartextTraffic" || echo " (not found)"
fi
doneRepository: team-aliens/DMS-Android
Length of output: 388
π Script executed:
#!/bin/bash
# View prod manifest with context around the security settings
echo "=== app/src/prod/AndroidManifest.xml (lines 10-30) ==="
sed -n '10,30p' app/src/prod/AndroidManifest.xmlRepository: team-aliens/DMS-Android
Length of output: 1030
π Script executed:
#!/bin/bash
# View main manifest application tag
echo "=== app/src/main/AndroidManifest.xml (lines 1-40) ==="
head -40 app/src/main/AndroidManifest.xmlRepository: team-aliens/DMS-Android
Length of output: 181
Remove insecure security settings from production manifest.
Both android:allowBackup="true" and android:usesCleartextTraffic="true" are enabled in the production manifest (lines 17, 24 of app/src/prod/AndroidManifest.xml). These settings pose security risks in production:
allowBackup="true"allows sensitive app data to be backed up to cloud storageusesCleartextTraffic="true"permits unencrypted HTTP traffic
These should be set to false in the production build to enforce HTTPS and prevent uncontrolled data backup.
π€ Prompt for AI Agents
In `@app/src/dev/AndroidManifest.xml` around lines 14 - 24, In the production
AndroidManifest change the insecure attributes so they are disabled: set
android:allowBackup="false" and android:usesCleartextTraffic="false" in the
<application> element (the manifest that currently contains
android:name=".android.app.DevApplication" / tools:replace="android:name");
update the production manifest's <application> attributes to explicitly use
these false values to prevent backups and block cleartext HTTP in production.
There was a problem hiding this comment.
devλ‘ λ§μ΄κ·Έλ μ΄μ ν μμ . skip
There was a problem hiding this comment.
@uson1004, understood! I'll skip this for now since you're planning to migrate to dev. Feel free to reach out if you need any assistance with the migration. π
...c/prod/kotlin/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSourceImpl.kt
Show resolved
Hide resolved
...evice/src/prod/kotlin/team/aliens/dms/android/core/device/datastore/store/DeviceStoreImpl.kt
Show resolved
Hide resolved
...evice/src/prod/kotlin/team/aliens/dms/android/core/device/datastore/store/DeviceStoreImpl.kt
Show resolved
Hide resolved
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .background(DmsTheme.colorScheme.background) | ||
| .systemBarsPadding() | ||
| .navigationBarsPadding(), | ||
| .statusBarsPadding(), | ||
| ) { |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
# First, let's check if the file exists and understand the codebase structure
find . -type f -name "HomeScreen.kt" | head -5Repository: team-aliens/DMS-Android
Length of output: 232
π Script executed:
# Get the file size to understand how much we need to read
wc -l feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.ktRepository: team-aliens/DMS-Android
Length of output: 152
π Script executed:
# Read the relevant section around lines 94-98 with more context
sed -n '80,120p' feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.ktRepository: team-aliens/DMS-Android
Length of output: 1347
π Script executed:
# Read more of the file to see the complete structure and bottom content
sed -n '100,140p' feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.ktRepository: team-aliens/DMS-Android
Length of output: 1526
π Script executed:
# Check the parent composable that calls HomeScreen
sed -n '1,93p' feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.ktRepository: team-aliens/DMS-Android
Length of output: 3707
π Script executed:
# Check if this Home composable is used within a Scaffold in parent files
rg -r "Home\(" --type kotlin | grep -E "(Scaffold|fun)" | head -20Repository: team-aliens/DMS-Android
Length of output: 463
π Script executed:
# Also check if there are navigation patterns or if this is integrated into a Scaffold elsewhere
rg "navigationBarsPadding" feature/src/dev/kotlin/team/aliens/dms/android/feature/main/ --type kotlin -B2 -A2Repository: team-aliens/DMS-Android
Length of output: 3506
π Script executed:
# Check if systemBarsPadding is a combined approach
rg "systemBarsPadding" feature/src/dev/kotlin/team/aliens/dms/android/ --type kotlin -B1 -A1 | head -30Repository: team-aliens/DMS-Android
Length of output: 3121
π Script executed:
# Check what systemBarsPadding does - it combines both status and navigation bar padding
# Let's see the complete pattern by checking if Home is wrapped in a Scaffold at navigation level
find . -type f -name "*.kt" -path "*/navigation/*" | xargs grep -l "Home" | head -5Repository: team-aliens/DMS-Android
Length of output: 253
π Script executed:
# Also check the prod version to see if it has different padding handling
sed -n '80,130p' feature/src/prod/kotlin/team/aliens/dms/android/feature/main/home/HomeScreen.ktRepository: team-aliens/DMS-Android
Length of output: 2281
π Script executed:
# Let's check the navigation route to understand the context
cat feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/navigation/HomeRoute.ktRepository: team-aliens/DMS-Android
Length of output: 948
π Script executed:
# Check if there's a parent scaffold handling insets
rg -A 10 "fun Home\(" feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.ktRepository: team-aliens/DMS-Android
Length of output: 479
π Script executed:
# Check if this Home composable is part of a Scaffold at a higher level
rg -B 5 "HomeRoute\(" feature/src/dev/kotlin/team/aliens/dms/android/feature/main/ --type kotlinRepository: team-aliens/DMS-Android
Length of output: 765
Add navigationBarsPadding to prevent bottom content overlap. The outer Column currently uses only .statusBarsPadding(), leaving action buttons at the bottom of the scrollable content exposed to gesture navigation bar overlap. The import for navigationBarsPadding already exists but is unused. Similar screens in the codebase (MyPageScreen, ApplicationScreen) use both top and bottom inset handling. Add .navigationBarsPadding() to the modifier chain to match the established pattern.
𧩠Suggested fix
modifier = Modifier
.fillMaxSize()
.background(DmsTheme.colorScheme.background)
- .statusBarsPadding(),
+ .statusBarsPadding()
+ .navigationBarsPadding(),π 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.
| modifier = Modifier | |
| .fillMaxSize() | |
| .background(DmsTheme.colorScheme.background) | |
| .systemBarsPadding() | |
| .navigationBarsPadding(), | |
| .statusBarsPadding(), | |
| ) { | |
| modifier = Modifier | |
| .fillMaxSize() | |
| .background(DmsTheme.colorScheme.background) | |
| .statusBarsPadding() | |
| .navigationBarsPadding(), | |
| ) { |
π€ Prompt for AI Agents
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/main/home/ui/HomeScreen.kt`
around lines 94 - 98, The outer Column in HomeScreen (the composable using
Modifier.fillMaxSize().background(DmsTheme.colorScheme.background).statusBarsPadding())
is missing bottom inset handling; update its modifier chain to also call
navigationBarsPadding() so the bottom action buttons aren't overlapped by
gesture/navigation barsβi.e., locate the Column in HomeScreen.kt and append
.navigationBarsPadding() to the existing modifier chain (preserving
.statusBarsPadding()) to match MyPageScreen/ApplicationScreen.
feature/src/dev/kotlin/team/aliens/dms/android/feature/notification/ui/component/NoticeItem.kt
Outdated
Show resolved
Hide resolved
| val pointHistoryList = remember(page, state.allPointList) { | ||
| when (tabData[page]) { | ||
| PointTab.All -> state.allPointList | ||
| PointTab.BONUS -> state.plusPointList | ||
| PointTab.Bonus -> state.bonusPointList | ||
| PointTab.Minus -> state.minusPointList |
There was a problem hiding this comment.
Fix stale list risk by removing or widening remember keys.
If bonusPointList or minusPointList updates without allPointList changing, the UI can keep showing stale data due to the current remember keys.
β Proposed fix (remove unnecessary remember)
- val pointHistoryList = remember(page, state.allPointList) {
- when (tabData[page]) {
- PointTab.All -> state.allPointList
- PointTab.Bonus -> state.bonusPointList
- PointTab.Minus -> state.minusPointList
- }
- }
+ val pointHistoryList =
+ when (tabData[page]) {
+ PointTab.All -> state.allPointList
+ PointTab.Bonus -> state.bonusPointList
+ PointTab.Minus -> state.minusPointList
+ }π€ Prompt for AI Agents
In
`@feature/src/dev/kotlin/team/aliens/dms/android/feature/point/ui/PointHistoryScreen.kt`
around lines 99 - 103, The current remember call that computes pointHistoryList
using keys (page, state.allPointList) can return stale data when
state.bonusPointList or state.minusPointList change; update the computation in
PointHistoryScreen by either removing the remember (compute pointHistoryList
directly) or expanding the remember keys to include state.bonusPointList and
state.minusPointList and tabData[page], so that pointHistoryList reacts to
changes; locate the remember invocation that assigns pointHistoryList and modify
it accordingly (symbols: pointHistoryList, remember, tabData,
PointTab.All/Bonus/Minus,
state.allPointList/state.bonusPointList/state.minusPointList).
feature/src/dev/kotlin/team/aliens/dms/android/feature/setting/viewmodel/SettingViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
π€ Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 37-40: In the "Create google-services.json" step, stop using
unquoted echo and the hardcoded absolute path; instead write the secret safely
with a quoted printf to preserve newlines/whitespace (use printf '%s\n' "$DATA")
and redirect into a path built from the $GITHUB_WORKSPACE variable (quote the
destination like "$GITHUB_WORKSPACE/.../app/google-services.json"), ensuring
DATA is referenced as "$DATA" and the destination is quoted for portability.
Updated the creation of google-services.json to use the workspace path and fixed the environment variable usage.
1 participant
κ°μ
μμ μ¬ν
μΆκ° λ‘ ν λ§
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores