[feat/#313] 알림창 UI 구현#320
Open
znayeonzn wants to merge 18 commits into
Open
Conversation
Walkthrough홈 화면에 알림 아이콘을 추가하고, 알림 데이터 모델·UI 상태·ViewModel·알림 화면(목록·헤더·탑바)·알림 카드 컴포넌트와 관련 드로어블 리소스를 도입합니다. 더미 알림 데이터가 포함됩니다. Changes알림 기능 전체 구현
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeScreen
participant NotificationRoute
participant NotificationViewModel
participant NotificationUiState
participant NotificationScreen
participant LazyColumn
participant NotificationCard
User->>HomeScreen: 홈 화면 표시
HomeScreen->>HomeScreen: hasNewNotification 확인
HomeScreen->>HomeScreen: 알림 아이콘 렌더링
User->>HomeScreen: 알림 아이콘 클릭
HomeScreen->>NotificationRoute: onNotificationIconClick 실행
NotificationRoute->>NotificationViewModel: hiltViewModel() 주입
NotificationViewModel->>NotificationUiState: uiState 제공
NotificationRoute->>NotificationScreen: uiState 전달
NotificationScreen->>NotificationScreen: TopBar/Header 렌더링
NotificationScreen->>LazyColumn: notificationList 확인
LazyColumn->>NotificationCard: 각 알림 항목 렌더링
User->>NotificationCard: 알림 카드 클릭
NotificationCard->>NotificationCard: landingLink 콜백 실행
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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. 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
🧹 Nitpick comments (1)
app/src/main/java/com/byeboo/app/presentation/notification/NotificationModel.kt (1)
6-13: ⚡ Quick win
notificationType를 문자열 대신 타입으로 고정하는 게 안전합니다.현재 구조는 오타/스키마 변경 시 컴파일 단계에서 잡히지 않고, 분기
else로 조용히 흘러갈 수 있습니다.enum class(또는 sealed type)로 바꿔서 모델-아이콘 매핑 계약을 타입으로 고정해 주세요.예시 diff
+enum class NotificationType { + QUEST_OPEN, + QUEST_EMPATHY, + QUEST_REPLY, +} + `@Immutable` data class NotificationModel( val notificationId: Long, - val notificationType: String, + val notificationType: NotificationType, val title: String, val content: String, val isRead: Boolean, val createdAt: String, val landingLink: String )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/byeboo/app/presentation/notification/NotificationModel.kt` around lines 6 - 13, Change NotificationModel.notificationType from String to a sealed/enum type (e.g., enum class NotificationType) and update all places that construct or consume NotificationModel to use NotificationType values instead of raw strings; add a mapping function or when expression over NotificationType (replacing any string-based icon mapping) so the compiler enforces exhaustive handling, and update your JSON (de)serialization rules (kotlinx.serialization/@Serializable or Gson/@SerializedName/TypeAdapter) to translate incoming string values into NotificationType, providing a safe fallback or Unknown enum entry for unrecognized values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/byeboo/app/presentation/home/HomeScreen.kt`:
- Line 113: The notification icon is currently a no-op (onNotificationIconClick
= {/*Todo: 알림*/ }), so wire it to the real navigation/action: replace the empty
lambda with a call to the notification navigation callback used by this screen
(invoke the provided onNotificationIconClick parameter or call your
navController's navigation function, e.g. navigateToNotifications()) inside
HomeScreen so clicking the icon triggers routing to the notifications screen;
ensure the HomeScreen signature exposes onNotificationIconClick (or add it) and
pass the correct lambda from the parent.
- Around line 198-199: The notification icon in the HomeScreen composable
currently sets contentDescription = null which prevents screen readers from
announcing its purpose; change that to a meaningful, localized description
(e.g., contentDescription =
stringResource(R.string.notification_icon_description)) used where the
Icon/IconButton is defined in HomeScreen (or the specific composable rendering
the notification icon), and add the corresponding string resource (e.g., "알림" or
"Notifications"); if you need more control, attach semantics via
Modifier.semantics { contentDescription = ... } instead of null.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.kt`:
- Around line 87-89: The clickable back icon in NotificationScreen (where
imageVector = ImageVector.vectorResource(id = R.drawable.ic_left)) currently
uses contentDescription = null which prevents screen readers from describing its
purpose; change it to provide a localized description (e.g., use stringResource
or R.string.back) and pass that string into the contentDescription for the Icon
(or IconButton) so the back action is accessible to assistive technologies.
- Around line 118-120: The header's "모두 읽기" click currently uses an empty lambda
in the noRippleClickable modifier, ignoring the passed-in onAllReadClick
parameter; update the call in NotificationScreen (the Modifier.noRippleClickable
block) to invoke the onAllReadClick callback instead of {} (and, if nullable,
guard or convert it to a no-op via onAllReadClick ?: {}) so the header uses the
provided onAllReadClick handler.
- Around line 46-47: The header action callbacks are no-ops: change the Route
that composes NotificationScreen to accept and forward real callbacks (e.g., add
parameters like onBackClick: () -> Unit and onAllReadClick: () -> Unit to the
Route function) and pass them into the NotificationScreen invocation instead of
the current placeholders; implement the route-side handlers to perform the
actual navigation/back action (e.g., call
navController.navigateUp()/popBackStack) and the “mark all read” behavior (call
the ViewModel method or repository function) so NotificationScreen's onBackClick
and onAllReadClick are wired to real logic.
- Around line 138-141: NotificationCard's onClick currently evaluates
notification.landingLink and discards it (onClick = { notification.landingLink
}), so replace the no-op lambda with a real invocation that forwards the landing
link to your navigation/handler; for example change onClick to call your
existing handler (e.g. onNotificationClick(notification.landingLink) or
navigateTo(notification.landingLink)) or pass the whole notification (e.g.
onClick = { onNotificationClick(notification) }) depending on the
NotificationCard/onClick signature so the click actually triggers the landing
behavior.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationUiState.kt`:
- Around line 6-9: NotificationUiState currently stores both hasNotification and
notificationList which can become inconsistent; change hasNotification from a
stored Boolean to a derived property computed from notificationList (e.g.,
hasNotification = notificationList.isNotEmpty()), remove the hasNotification
constructor parameter/field from the data class, update the NotificationUiState
declaration (and any callers/constructors/tests) to accept only
notificationList, and update all usages to rely on the new derived
hasNotification accessor on NotificationUiState.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationViewModel.kt`:
- Line 12: The initial UI state uses NotificationUiState(false,
dummyNotifications) which mismatches the provided dummyNotifications and
prevents rendering; update the NotificationViewModel initialization of _uiState
so hasNotification reflects the actual list (e.g., set hasNotification =
dummyNotifications.isNotEmpty() or true when dummyNotifications contains items)
to keep NotificationUiState consistent with dummyNotifications.
---
Nitpick comments:
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationModel.kt`:
- Around line 6-13: Change NotificationModel.notificationType from String to a
sealed/enum type (e.g., enum class NotificationType) and update all places that
construct or consume NotificationModel to use NotificationType values instead of
raw strings; add a mapping function or when expression over NotificationType
(replacing any string-based icon mapping) so the compiler enforces exhaustive
handling, and update your JSON (de)serialization rules
(kotlinx.serialization/@Serializable or Gson/@SerializedName/TypeAdapter) to
translate incoming string values into NotificationType, providing a safe
fallback or Unknown enum entry for unrecognized values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b64fda5-3eb0-407c-865e-7cdffaffa23d
📒 Files selected for processing (13)
app/src/main/java/com/byeboo/app/presentation/home/HomeScreen.ktapp/src/main/java/com/byeboo/app/presentation/home/HomeUiState.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationCard.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationModel.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationUiState.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationViewModel.ktapp/src/main/java/com/byeboo/app/presentation/quest/util/NotificationTypeIcon.ktapp/src/main/res/drawable/ic_new_notification.xmlapp/src/main/res/drawable/ic_notification.xmlapp/src/main/res/drawable/ic_notification_quest.xmlapp/src/main/res/drawable/ic_notification_reaction.xmlgradle/libs.versions.toml
| paddingValues = paddingValues, | ||
| onOffboardingNewJourneyClick = viewModel::onOffboardingNewJourneyClicked, | ||
| onLottieClick = viewModel::onLottieClicked, | ||
| onNotificationIconClick = {/*Todo: 알림*/ } |
There was a problem hiding this comment.
홈 알림 아이콘 클릭이 현재 no-op입니다.
아이콘이 렌더링되지만 실제 네비게이션/액션으로 연결되지 않아 기능이 닫혀 있습니다. 라우팅 콜백을 연결해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/byeboo/app/presentation/home/HomeScreen.kt` at line
113, The notification icon is currently a no-op (onNotificationIconClick =
{/*Todo: 알림*/ }), so wire it to the real navigation/action: replace the empty
lambda with a call to the notification navigation callback used by this screen
(invoke the provided onNotificationIconClick parameter or call your
navController's navigation function, e.g. navigateToNotifications()) inside
HomeScreen so clicking the icon triggers routing to the notifications screen;
ensure the HomeScreen signature exposes onNotificationIconClick (or add it) and
pass the correct lambda from the parent.
Comment on lines
+198
to
+199
| contentDescription = null, | ||
| tint = Color.Unspecified, |
There was a problem hiding this comment.
탭 가능한 알림 아이콘에 접근성 설명을 추가해 주세요.
contentDescription = null로 두면 보조기기에서 버튼 의미를 읽을 수 없습니다.
예시 diff
Icon(
@@
- contentDescription = null,
+ contentDescription = "알림",
tint = Color.Unspecified,📝 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.
Suggested change
| contentDescription = null, | |
| tint = Color.Unspecified, | |
| contentDescription = "알림", | |
| tint = Color.Unspecified, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/byeboo/app/presentation/home/HomeScreen.kt` around
lines 198 - 199, The notification icon in the HomeScreen composable currently
sets contentDescription = null which prevents screen readers from announcing its
purpose; change that to a meaningful, localized description (e.g.,
contentDescription = stringResource(R.string.notification_icon_description))
used where the Icon/IconButton is defined in HomeScreen (or the specific
composable rendering the notification icon), and add the corresponding string
resource (e.g., "알림" or "Notifications"); if you need more control, attach
semantics via Modifier.semantics { contentDescription = ... } instead of null.
Comment on lines
+46
to
+47
| onBackClick = { /*Todo : back action */ }, | ||
| onAllReadClick = {}, |
There was a problem hiding this comment.
상단 액션 콜백이 실제 동작에 연결되어 있지 않습니다.
onBackClick/onAllReadClick이 no-op으로 고정되어 있어 상단 인터랙션이 동작하지 않습니다. Route에서 콜백을 파라미터로 받아 연결해 주세요.
예시 diff
fun NotificationRoute(
modifier: Modifier = Modifier,
paddingValues: PaddingValues,
+ onBackClick: () -> Unit,
+ onAllReadClick: () -> Unit,
viewModel: NotificationViewModel = hiltViewModel()
) {
@@
NotificationScreen(
uiState = uiState,
paddingValues = paddingValues,
- onBackClick = { /*Todo : back action */ },
- onAllReadClick = {},
+ onBackClick = onBackClick,
+ onAllReadClick = onAllReadClick,
modifier = modifier
)
}📝 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.
Suggested change
| onBackClick = { /*Todo : back action */ }, | |
| onAllReadClick = {}, | |
| fun NotificationRoute( | |
| modifier: Modifier = Modifier, | |
| paddingValues: PaddingValues, | |
| onBackClick: () -> Unit, | |
| onAllReadClick: () -> Unit, | |
| viewModel: NotificationViewModel = hiltViewModel() | |
| ) { | |
| val uiState by viewModel.uiState.collectAsStateWithLifecycle() | |
| NotificationScreen( | |
| uiState = uiState, | |
| paddingValues = paddingValues, | |
| onBackClick = onBackClick, | |
| onAllReadClick = onAllReadClick, | |
| modifier = modifier | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.kt`
around lines 46 - 47, The header action callbacks are no-ops: change the Route
that composes NotificationScreen to accept and forward real callbacks (e.g., add
parameters like onBackClick: () -> Unit and onAllReadClick: () -> Unit to the
Route function) and pass them into the NotificationScreen invocation instead of
the current placeholders; implement the route-side handlers to perform the
actual navigation/back action (e.g., call
navController.navigateUp()/popBackStack) and the “mark all read” behavior (call
the ViewModel method or repository function) so NotificationScreen's onBackClick
and onAllReadClick are wired to real logic.
Comment on lines
+87
to
+89
| imageVector = ImageVector.vectorResource(id = R.drawable.ic_left), | ||
| contentDescription = null, | ||
| tint = ByeBooTheme.colors.gray50, |
There was a problem hiding this comment.
클릭 가능한 뒤로가기 아이콘에 접근성 설명이 필요합니다.
탭 가능한 아이콘인데 contentDescription = null이면 스크린리더에서 목적을 전달하지 못합니다.
예시 diff
Icon(
imageVector = ImageVector.vectorResource(id = R.drawable.ic_left),
- contentDescription = null,
+ contentDescription = "뒤로가기",
tint = ByeBooTheme.colors.gray50,📝 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.
Suggested change
| imageVector = ImageVector.vectorResource(id = R.drawable.ic_left), | |
| contentDescription = null, | |
| tint = ByeBooTheme.colors.gray50, | |
| imageVector = ImageVector.vectorResource(id = R.drawable.ic_left), | |
| contentDescription = "뒤로가기", | |
| tint = ByeBooTheme.colors.gray50, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.kt`
around lines 87 - 89, The clickable back icon in NotificationScreen (where
imageVector = ImageVector.vectorResource(id = R.drawable.ic_left)) currently
uses contentDescription = null which prevents screen readers from describing its
purpose; change it to provide a localized description (e.g., use stringResource
or R.string.back) and pass that string into the contentDescription for the Icon
(or IconButton) so the back action is accessible to assistive technologies.
Comment on lines
+118
to
+120
| modifier = Modifier.noRippleClickable( | ||
| onClick = {} //Todo : 클릭 시 동작 | ||
| ) |
There was a problem hiding this comment.
onAllReadClick 파라미터가 무시되고 있습니다.
헤더에서 전달받은 콜백 대신 빈 람다를 사용하고 있어 “모두 읽기”가 항상 no-op입니다.
예시 diff
Text(
text = "모두 읽기",
@@
modifier = Modifier.noRippleClickable(
- onClick = {} //Todo : 클릭 시 동작
+ onClick = onAllReadClick
)
)📝 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.
Suggested change
| modifier = Modifier.noRippleClickable( | |
| onClick = {} //Todo : 클릭 시 동작 | |
| ) | |
| modifier = Modifier.noRippleClickable( | |
| onClick = onAllReadClick | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.kt`
around lines 118 - 120, The header's "모두 읽기" click currently uses an empty
lambda in the noRippleClickable modifier, ignoring the passed-in onAllReadClick
parameter; update the call in NotificationScreen (the Modifier.noRippleClickable
block) to invoke the onAllReadClick callback instead of {} (and, if nullable,
guard or convert it to a no-op via onAllReadClick ?: {}) so the header uses the
provided onAllReadClick handler.
Comment on lines
+138
to
+141
| NotificationCard( | ||
| notification = notification, | ||
| onClick = { notification.landingLink } | ||
| ) |
There was a problem hiding this comment.
알림 카드 클릭 콜백이 실질적으로 아무 동작도 하지 않습니다.
현재 람다는 전달 인자를 소비하지 않고 문자열만 평가해서 버립니다. 실제 랜딩 처리 콜백에 연결해 주세요.
예시 diff
items(
items = uiState.notificationList,
key = { it.notificationId },
) { notification ->
NotificationCard(
notification = notification,
- onClick = { notification.landingLink }
+ onClick = { landingLink ->
+ // TODO: navigate/open landingLink
+ }
)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.kt`
around lines 138 - 141, NotificationCard's onClick currently evaluates
notification.landingLink and discards it (onClick = { notification.landingLink
}), so replace the no-op lambda with a real invocation that forwards the landing
link to your navigation/handler; for example change onClick to call your
existing handler (e.g. onNotificationClick(notification.landingLink) or
navigateTo(notification.landingLink)) or pass the whole notification (e.g.
onClick = { onNotificationClick(notification) }) depending on the
NotificationCard/onClick signature so the click actually triggers the landing
behavior.
Comment on lines
+6
to
+9
| data class NotificationUiState( | ||
| val hasNotification: Boolean = false, | ||
| val notificationList: ImmutableList<NotificationModel> = persistentListOf(), | ||
| ) No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
hasNotification는 파생 상태로 계산하는 형태가 안전합니다.
hasNotification와 notificationList를 동시에 들고 있으면 불일치가 쉽게 발생합니다. notificationList.isNotEmpty()로 파생시키는 구조로 바꾸는 걸 권장합니다.
예시 diff
data class NotificationUiState(
- val hasNotification: Boolean = false,
val notificationList: ImmutableList<NotificationModel> = persistentListOf(),
-)
+) {
+ val hasNotification: Boolean
+ get() = notificationList.isNotEmpty()
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationUiState.kt`
around lines 6 - 9, NotificationUiState currently stores both hasNotification
and notificationList which can become inconsistent; change hasNotification from
a stored Boolean to a derived property computed from notificationList (e.g.,
hasNotification = notificationList.isNotEmpty()), remove the hasNotification
constructor parameter/field from the data class, update the NotificationUiState
declaration (and any callers/constructors/tests) to accept only
notificationList, and update all usages to rely on the new derived
hasNotification accessor on NotificationUiState.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| @HiltViewModel | ||
| class NotificationViewModel @Inject constructor() : ViewModel() { | ||
| private val _uiState = MutableStateFlow(NotificationUiState(false,dummyNotifications)) |
There was a problem hiding this comment.
초기 상태가 데이터와 불일치해서 목록이 렌더링되지 않습니다.
dummyNotifications가 비어있지 않은데 hasNotification = false로 초기화되어, 화면이 빈 상태로 고정됩니다.
예시 diff
- private val _uiState = MutableStateFlow(NotificationUiState(false,dummyNotifications))
+ private val _uiState = MutableStateFlow(
+ NotificationUiState(
+ hasNotification = true,
+ notificationList = dummyNotifications,
+ ),
+ )📝 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.
Suggested change
| private val _uiState = MutableStateFlow(NotificationUiState(false,dummyNotifications)) | |
| private val _uiState = MutableStateFlow( | |
| NotificationUiState( | |
| hasNotification = true, | |
| notificationList = dummyNotifications, | |
| ), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationViewModel.kt`
at line 12, The initial UI state uses NotificationUiState(false,
dummyNotifications) which mismatches the provided dummyNotifications and
prevents rendering; update the NotificationViewModel initialization of _uiState
so hasNotification reflects the actual list (e.g., set hasNotification =
dummyNotifications.isNotEmpty() or true when dummyNotifications contains items)
to keep NotificationUiState consistent with dummyNotifications.
sohee6989
approved these changes
May 23, 2026
| val landingLink: String | ||
| ) | ||
|
|
||
| val dummyNotifications = persistentListOf( |
Collaborator
Author
There was a problem hiding this comment.
넵! 서버 연결할 때 지우겠습니다!
Comment on lines
+36
to
+37
| modifier: Modifier = Modifier, | ||
| paddingValues: PaddingValues, |
Comment on lines
+9
to
+10
| "QUEST_OPEN" -> R.drawable.ic_notification_quest | ||
| "COMMENT", "LIKE" -> R.drawable.ic_notification_reaction |
Collaborator
There was a problem hiding this comment.
p1:이거 타입으로 관리해주는거 어떠세요??
지금 String으로 관리되고 있어서 오타 발생해도 컴파일 타임에 확인이 어려워서요!
Comment on lines
+6
to
+9
| data class NotificationUiState( | ||
| val hasNotification: Boolean = false, | ||
| val notificationList: ImmutableList<NotificationModel> = persistentListOf(), | ||
| ) No newline at end of file |
|
|
||
| @HiltViewModel | ||
| class NotificationViewModel @Inject constructor() : ViewModel() { | ||
| private val _uiState = MutableStateFlow(NotificationUiState(false,dummyNotifications)) |
Collaborator
There was a problem hiding this comment.
p2: 이거 더미값 빼주고 머지해주세요~!
| ) { | ||
| Row( | ||
| horizontalArrangement = Arrangement.spacedBy(screenWidthDp(4.dp)), | ||
| verticalAlignment = Alignment.CenterVertically |
Collaborator
There was a problem hiding this comment.
p1: 이거 top으로 해줘야되요!
피그마 보면 중앙정렬이 아니라 아이콘 상단 기준으로 텍스트가 배치되어 있어서요!
| Box( | ||
| modifier = Modifier.fillMaxSize(), | ||
| contentAlignment = Alignment.Center | ||
|
|
Collaborator
There was a problem hiding this comment.
p2: 공배 제거해주세요오~ㅎㅎ
머지전에 린트도 부탁드립니다!!
| } else { | ||
| Box( | ||
| modifier = Modifier.fillMaxSize(), | ||
| contentAlignment = Alignment.Center |
Collaborator
There was a problem hiding this comment.
p2: 이거 center보다는 공백 비율을 weight로 넣어줘야지 피그마상 화면느낌처럼 나올것 같아요!
| .padding( | ||
| top = paddingValues.calculateTopPadding() + screenHeightDp(43.dp), | ||
| bottom = paddingValues.calculateBottomPadding()) | ||
| .padding(horizontal = screenWidthDp(24.dp)), |
Collaborator
There was a problem hiding this comment.
p1: 지금 가로패딩 들어간 부분에서 스크롤이 안 되고 있어서 이거 수정해주세요!
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.kt`:
- Around line 131-139: NotificationScreen's LazyColumn ignores the passed-in
modifier so callers' layout constraints like weight(1f) are lost; update the
LazyColumn (and any other list branches in NotificationScreen) to apply the
incoming modifier by using modifier = modifier.padding(...) or
modifier.then(Modifier.padding(...)) instead of the current hardcoded
PaddingValues only, ensuring the passed modifier (e.g., weight(1f)) is respected
across list states.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 548b0229-4a9b-4fcb-a95a-2b9cb6e9895e
📒 Files selected for processing (9)
app/src/main/java/com/byeboo/app/presentation/home/HomeScreen.ktapp/src/main/java/com/byeboo/app/presentation/home/HomeUiState.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationCard.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationModel.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationType.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationUiState.ktapp/src/main/java/com/byeboo/app/presentation/notification/NotificationViewModel.ktapp/src/main/java/com/byeboo/app/presentation/quest/util/NotificationTypeIcon.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/byeboo/app/presentation/notification/NotificationType.kt
Comment on lines
+131
to
+139
| LazyColumn( | ||
| contentPadding = | ||
| PaddingValues( | ||
| start = screenWidthDp(24.dp), | ||
| end = screenWidthDp(24.dp), | ||
| bottom = screenHeightDp(24.dp), | ||
| ), | ||
| verticalArrangement = Arrangement.spacedBy(screenHeightDp(20.dp)), | ||
| ) { |
There was a problem hiding this comment.
리스트 분기에서도 전달된 modifier를 적용해 레이아웃 제약을 일관화해주세요.
Line 131의 LazyColumn이 modifier를 받지 않아, NotificationScreen에서 넘긴 weight(1f)가 목록 상태에서는 무시됩니다. 이로 인해 상태별 스크롤/배치가 달라질 수 있습니다.
수정 예시
- LazyColumn(
+ LazyColumn(
+ modifier = modifier.fillMaxWidth(),
contentPadding =
PaddingValues(
start = screenWidthDp(24.dp),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/byeboo/app/presentation/notification/NotificationScreen.kt`
around lines 131 - 139, NotificationScreen's LazyColumn ignores the passed-in
modifier so callers' layout constraints like weight(1f) are lost; update the
LazyColumn (and any other list branches in NotificationScreen) to apply the
incoming modifier by using modifier = modifier.padding(...) or
modifier.then(Modifier.padding(...)) instead of the current hardcoded
PaddingValues only, ensuring the passed modifier (e.g., weight(1f)) is respected
across list states.
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.
Related issue 🛠
Work Description 📝
Screenshot 📸
default.mp4
Uncompleted Tasks 😅
PR Point 📌
트러블 슈팅 💥
Summary by CodeRabbit
새로운 기능
기타