SCRUM-283 design: Add notification screen - #67
Conversation
Create NotificationScreen and NotificationUiState to structure the layout of the notification area. Introduce NotificationListItem and NotificationEmptyState for displaying individual alerts and empty states.
Register the notification screen route in the navigation graph and update the home screen notification click handler to navigate to NotificationScreen.
Integrate NotificationViewModel to manage tab selection and filter notification items. Refactor NotificationListItem to load image URLs using AsyncImage and display an unread badge on top-end of the image. Update NotificationScreen to use a top bar with a back button and show dividers between items. Add unit and UI tests for verification.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a notifications destination with Compose UI, tab-driven ViewModel state, empty and populated views, read indicators, deleted-note dialog handling, navigation wiring, previews, and unit/instrumentation tests. ChangesNotification feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b81715b522
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lyrics/feelin/presentation/view/notification/component/NotificationListItem.kt`:
- Around line 37-42: Reorder the parameters of the composable
NotificationListItem so the optional modifier parameter comes before the onClick
lambda, while preserving their types, defaults, and existing behavior to enable
trailing-lambda call syntax.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/notification/NotificationScreen.kt`:
- Around line 32-40: Reorder NotificationScreen parameters so modifier is the
first optional parameter after the required uiState, followed by the lambda
parameters, while preserving their names and types to keep named callers
compatible.
🪄 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 Plus
Run ID: db71aa5d-71c4-436c-905a-1562f3af376a
📒 Files selected for processing (11)
app/src/androidTest/java/com/lyrics/feelin/presentation/view/notification/NotificationScreenTest.ktapp/src/androidTest/java/com/lyrics/feelin/presentation/view/notification/component/NotificationListItemTest.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinDestination.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.ktapp/src/main/java/com/lyrics/feelin/navigation/HomeNavigation.ktapp/src/main/java/com/lyrics/feelin/presentation/view/notification/NotificationScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/notification/NotificationUiState.ktapp/src/main/java/com/lyrics/feelin/presentation/view/notification/NotificationViewModel.ktapp/src/main/java/com/lyrics/feelin/presentation/view/notification/component/NotificationEmptyState.ktapp/src/main/java/com/lyrics/feelin/presentation/view/notification/component/NotificationListItem.ktapp/src/test/java/com/lyrics/feelin/presentation/view/notification/NotificationViewModelTest.kt
|
@codex re-review this except latest review comments. |
|
@coderabbitai re-review this except latest review comments. |
|
The PR description images show four screenshots of the notification screen in the Feelin app:
These confirm the notification screen's tabs, list items, unread indicators, and empty states render correctly in both light and dark modes as described in the PR objectives.
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b81715b522
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
병합하겠습니다. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
What is the current behavior?
홈 화면의 알림 아이콘에 이동 동작이 연결되어 있지 않고, 알림 목록을 보여주는 화면과 Navigation 목적지가 없습니다.
What is the new behavior (if this is a feature change)?
알림 화면을 구현하고 홈 화면 및 Navigation graph에 연결했습니다.
내 소식,전체탭과 탭별 샘플 상태NotificationViewModel과StateFlow기반 상태 관리서버 및 Repository를 통한 실제 알림 조회와 알림 항목 클릭 후 상세 화면 이동은 이번 작업 범위에서 제외했습니다.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
아니요. 기존 기능에 영향을 주는 breaking change는 없습니다.
ScreenShots (If needed)
Light mode
Dark mode
Other information:
AI Agent
다음 항목을 검증했습니다.
./gradlew :app:testDevDebugUnitTest./gradlew :app:connectedDevDebugAndroidTest(15 tests)./gradlew detekt./gradlew :app:assembleDebugUser
지금은 알림 화면 이동이 홈 화면에서만 있는데, 실제 구현에서는 마이페이지에서의 이동과 아티스트 화면에서의 이동(요구사항이 약간 다름)이 있습니다. 이는 실제 기능 연동시에 같이 연동하겠습니다.
Summary by CodeRabbit
New Features
Tests