SCRUM-281 design: add like artist screen#63
Merged
gdaegeun539 merged 8 commits intoJul 5, 2026
Merged
Conversation
Implement two bottom sheets for managing favorite artists on the home screen: one to view all favorite artists, and another to search and add more artists with interactive search filtering. - Add MyFavoriteArtistsBottomSheet to list user's favorite artists - Add SearchMoreFavoriteArtistBottomSheet with search debounce support - Integrate bottom sheets into HomeScreen with state updates in HomeViewModel - Add corresponding unit and instrumentation tests
Introduce HomeBottomSheetScaffold with a rectangular shape and no drag handle to meet the home screen's design requirements. Replace FeelinModalBottomSheet with the new scaffold in home-related sheets.
…Scaffold Move full height modifier and status bar padding logic from individual bottom sheets to the shared HomeBottomSheetScaffold component to prevent content overlap with the status bar.
Remove custom duplicate item composables (MyFavoriteArtistItem, SearchArtistItem) in favor of the shared ArtistBubbleComponent. Add the id property to ArtistBubbleComponentData.FavoriteArtistFindType to support artist click handlers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds home bottom-sheet state and wiring, two favorite-artist bottom-sheet implementations with their view models and tests, a shared bottom-sheet scaffold, artist bubble data updates, and a new gray design-system button. ChangesHome Favorite Artist Bottom Sheets
FeelinGrayButton design system component
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant HomeScreen
participant HomeViewModel
participant MyFavoriteArtistsBottomSheet
participant SearchMoreFavoriteArtistBottomSheet
User->>HomeScreen: tap "show all" / "search more"
HomeScreen->>HomeViewModel: showMyFavoriteArtistsBottomSheet() / showSearchMoreArtistsBottomSheet()
HomeViewModel-->>HomeScreen: uiState.bottomSheetType updated
alt MyFavoriteArtists
HomeScreen->>MyFavoriteArtistsBottomSheet: render(artists)
User->>MyFavoriteArtistsBottomSheet: tap artist
MyFavoriteArtistsBottomSheet-->>HomeScreen: onDismissRequest() + onArtistClick(id)
else SearchMoreArtists
HomeScreen->>SearchMoreFavoriteArtistBottomSheet: render()
User->>SearchMoreFavoriteArtistBottomSheet: type query / tap artist
SearchMoreFavoriteArtistBottomSheet-->>HomeScreen: onDismissRequest() + onArtistClick(id)
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheet.kt (1)
89-98: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winContent description language mismatch.
"Close"here vs."닫기"inMyFavoriteArtistsBottomSheetfor the same icon/action. Consolidate to one consistent language for accessibility.🤖 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/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheet.kt` around lines 89 - 98, The close icon content description in SearchMoreFavoriteArtistBottomSheet is using a different language than the same action in MyFavoriteArtistsBottomSheet, so make them consistent for accessibility. Update the Icon contentDescription in the SearchMoreFavoriteArtistBottomSheet IconButton to match the wording used elsewhere for the close action, and keep the label consistent across both bottom sheet components.
🧹 Nitpick comments (5)
app/src/main/java/com/lyrics/feelin/presentation/view/component/artist/ArtistBubbleComponentData.kt (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent id nullability across sealed variants.
FavoriteArtistFindType.idusesLong = 0Lwhile the siblingHomeFavoriteArtistType.id(line 19) usesLong? = null. Mixing sentinel-default and nullable-default patterns for the same conceptual field within one sealed interface is inconsistent and risks treating a real id of0Las "unset" downstream.♻️ Suggested consistency fix
data class FavoriteArtistFindType( override val name: String, val imageUrl: String, - val id: Long = 0L, + val id: Long? = null, ) : ArtistBubbleComponentData🤖 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/lyrics/feelin/presentation/view/component/artist/ArtistBubbleComponentData.kt` around lines 28 - 32, Make the id handling consistent across the ArtistBubbleComponentData sealed variants by aligning FavoriteArtistFindType.id with HomeFavoriteArtistType.id. Update FavoriteArtistFindType to use the same nullable/default pattern as the sibling type and adjust any call sites or comparisons that assume 0L means unset, so downstream code treats the conceptual field uniformly.app/src/main/java/com/lyrics/feelin/core/designsystem/component/FeelinGrayButton.kt (1)
19-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing
enabledfor reusability.As a shared design-system component,
FeelinGrayButtonmay benefit from anenabled: Boolean = trueparameter passed through toButton, so future consumers can disable it without duplicating the composable.♻️ Optional enhancement
fun FeelinGrayButton( text: String, onClick: () -> Unit, modifier: Modifier = Modifier, + enabled: Boolean = true, ) { val feelinColors = LocalFeelinColors.current Button( onClick = onClick, modifier = modifier, + enabled = enabled, shape = RoundedCornerShape(8.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/lyrics/feelin/core/designsystem/component/FeelinGrayButton.kt` around lines 19 - 41, Add an enabled parameter to FeelinGrayButton so it can be reused in disabled states without duplicating the composable. Update the FeelinGrayButton signature to accept enabled: Boolean = true and pass it through to the underlying Button call, keeping existing defaults and behavior unchanged for current callers.app/src/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheetTest.kt (1)
15-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for search/filter behavior.
The test only checks static strings render; it doesn't exercise typing a query, the debounced filtering, or the empty-state vs. grid toggle — the main new logic in this feature.
🤖 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/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheetTest.kt` around lines 15 - 31, The current SearchMoreFavoriteArtistBottomSheetTest only verifies static text, so add coverage for the new search flow in SearchMoreFavoriteArtistBottomSheet and SearchMoreFavoriteArtistViewModel. Update the test to type into the search field, assert the debounced filtered results update, and verify the UI switches between the empty state and the artist grid as the query changes. Use the existing composables and test setup in SearchMoreFavoriteArtistBottomSheetTest to locate the search input, then assert the behavior driven by the view model’s filtering state.app/src/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheetTest.kt (1)
16-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing click behavior, not just text rendering.
The test verifies displayed text but not that tapping an artist bubble calls
onArtistClickwith the correct id and dismisses the sheet — the actual interactive contract of this composable.🤖 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/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheetTest.kt` around lines 16 - 46, The current MyFavoriteArtistsBottomSheet test only checks that the title and artist names render, but it misses the composable’s interaction contract. Update MyFavoriteArtistsBottomSheetTest.sheetDisplaysTitleAndArtistNames to also tap an artist bubble and verify the onArtistClick callback receives the correct artist id and that onDismissRequest is invoked. Use the existing MyFavoriteArtistsBottomSheet composable and the artist data setup in the test to locate the clickable nodes and assert the callback behavior.app/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheet.kt (1)
86-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing stable
keyinLazyVerticalGriditems; nullable-id fallback may collide.Two related concerns:
items(artists) { ... }has nokey, unlikeSearchMoreFavoriteArtistBottomSheet'sitems(artists, key = { it.id }). Adding a key avoids unnecessary recomposition/animation glitches on list changes.id = artist.id ?: 0L(line 97) silently maps every artist with a null id to0Lfor display, while the click handler correctly no-ops for null ids (line 101). If more than one artist lacks an id, they'd all render with the same synthetic id (0L), and could collide with a real artist whose id is0.Since `ArtistBubbleComponentData` internals aren't in this review's file set, please confirm whether `id` is used for anything beyond display (e.g., keying/animation) in `ArtistBubbleComponent`.♻️ Suggested fix
- items(artists) { artist -> + items(artists, key = { it.id ?: -1L }) { artist ->🤖 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/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheet.kt` around lines 86 - 105, The LazyVerticalGrid in MyFavoriteArtistsBottomSheet uses items(artists) without a stable key, and the FavoriteArtistFindType id fallback to 0L can collide for null IDs. Update the items call to supply a key based on artist.id (or another stable unique value) similar to SearchMoreFavoriteArtistBottomSheet, and avoid mapping null ids to 0L in ArtistBubbleComponentData if that id is used for identity, animation, or selection; keep nulls distinct or confirm the component only uses it for display.
🤖 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/home/favorite/MyFavoriteArtistsBottomSheet.kt`:
- Around line 64-73: The close icon’s contentDescription is inconsistent across
similar bottom sheets, so make `MyFavoriteArtistsBottomSheet` match the app’s
Korean accessibility copy. Update the `Icon` inside `IconButton` in this sheet
to use a Korean label consistent with the rest of the file and align it with the
same `CloseIcon` usage pattern found in `SearchMoreFavoriteArtistBottomSheet`.
- Around line 37-42: The Composable parameter order in
MyFavoriteArtistsBottomSheet is violating the lambda-last convention because the
optional viewModel follows onArtistClick and onDismissRequest. Reorder the
parameters so all lambda parameters come last, and apply the same fix in
SearchMoreFavoriteArtistBottomSheet to keep the Composable API consistent with
the project rule.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheet.kt`:
- Around line 53-58: The parameter order in SearchMoreFavoriteArtistBottomSheet
violates the composable convention because the lambda parameters are not last.
Reorder the function signature so onArtistClick and onDismissRequest come after
modifier and viewModel, matching the pattern used by
MyFavoriteArtistsBottomSheet and the composable lambda-last rule in this
package.
- Around line 148-157: The “아티스트 요청하기” button in
SearchMoreFavoriteArtistBottomSheet currently launches an ACTION_VIEW intent
directly and can still fail with ActivityNotFoundException. Update the onClick
logic to use the existing Context.openExternalBrowser() helper instead of
context.startActivity(intent), so the fallback toast behavior is reused
consistently.
---
Duplicate comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheet.kt`:
- Around line 89-98: The close icon content description in
SearchMoreFavoriteArtistBottomSheet is using a different language than the same
action in MyFavoriteArtistsBottomSheet, so make them consistent for
accessibility. Update the Icon contentDescription in the
SearchMoreFavoriteArtistBottomSheet IconButton to match the wording used
elsewhere for the close action, and keep the label consistent across both bottom
sheet components.
---
Nitpick comments:
In
`@app/src/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheetTest.kt`:
- Around line 16-46: The current MyFavoriteArtistsBottomSheet test only checks
that the title and artist names render, but it misses the composable’s
interaction contract. Update
MyFavoriteArtistsBottomSheetTest.sheetDisplaysTitleAndArtistNames to also tap an
artist bubble and verify the onArtistClick callback receives the correct artist
id and that onDismissRequest is invoked. Use the existing
MyFavoriteArtistsBottomSheet composable and the artist data setup in the test to
locate the clickable nodes and assert the callback behavior.
In
`@app/src/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheetTest.kt`:
- Around line 15-31: The current SearchMoreFavoriteArtistBottomSheetTest only
verifies static text, so add coverage for the new search flow in
SearchMoreFavoriteArtistBottomSheet and SearchMoreFavoriteArtistViewModel.
Update the test to type into the search field, assert the debounced filtered
results update, and verify the UI switches between the empty state and the
artist grid as the query changes. Use the existing composables and test setup in
SearchMoreFavoriteArtistBottomSheetTest to locate the search input, then assert
the behavior driven by the view model’s filtering state.
In
`@app/src/main/java/com/lyrics/feelin/core/designsystem/component/FeelinGrayButton.kt`:
- Around line 19-41: Add an enabled parameter to FeelinGrayButton so it can be
reused in disabled states without duplicating the composable. Update the
FeelinGrayButton signature to accept enabled: Boolean = true and pass it through
to the underlying Button call, keeping existing defaults and behavior unchanged
for current callers.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/component/artist/ArtistBubbleComponentData.kt`:
- Around line 28-32: Make the id handling consistent across the
ArtistBubbleComponentData sealed variants by aligning FavoriteArtistFindType.id
with HomeFavoriteArtistType.id. Update FavoriteArtistFindType to use the same
nullable/default pattern as the sibling type and adjust any call sites or
comparisons that assume 0L means unset, so downstream code treats the conceptual
field uniformly.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheet.kt`:
- Around line 86-105: The LazyVerticalGrid in MyFavoriteArtistsBottomSheet uses
items(artists) without a stable key, and the FavoriteArtistFindType id fallback
to 0L can collide for null IDs. Update the items call to supply a key based on
artist.id (or another stable unique value) similar to
SearchMoreFavoriteArtistBottomSheet, and avoid mapping null ids to 0L in
ArtistBubbleComponentData if that id is used for identity, animation, or
selection; keep nulls distinct or confirm the component only uses it for
display.
🪄 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: f3b5415e-4454-42f2-9ecf-cdee262ec6d7
📒 Files selected for processing (15)
app/src/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheetTest.ktapp/src/androidTest/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheetTest.ktapp/src/main/java/com/lyrics/feelin/core/designsystem/component/FeelinGrayButton.ktapp/src/main/java/com/lyrics/feelin/presentation/view/component/artist/ArtistBubbleComponentData.ktapp/src/main/java/com/lyrics/feelin/presentation/view/component/artist/ArtistProfileComponent.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/HomeScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/HomeUiState.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/HomeViewModel.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/component/HomeBottomSheetScaffold.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsBottomSheet.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/MyFavoriteArtistsViewModel.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistBottomSheet.ktapp/src/main/java/com/lyrics/feelin/presentation/view/home/favorite/SearchMoreFavoriteArtistViewModel.ktapp/src/test/java/com/lyrics/feelin/presentation/view/home/HomeUiStateTest.ktapp/src/test/java/com/lyrics/feelin/presentation/view/home/HomeViewModelTest.kt
Move onDismissRequest parameter to the end with a default value to match Compose conventions. Clean up imports and use openExternalBrowser extension for Google Form link. Update close icon content description.
Member
Author
병합하겠습니다. Codex 리뷰는 첫 리뷰에 통과했습니다. |
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
1 participant
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.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
What is the current behavior?
홈화면의 관심 아티스트를 모아보는 화면과 전체 아티스트를 조회하는 화면이 추가되지 않았습니다.
What is the new behavior (if this is a feature change)?
아래 두개의 화면을 구현합니다.
iOS 앱의 구현과 다르게, 이 화면들을 모달 다이얼로그 형태로 구현했습니다. 스크롤로 이 창들을 닫을 수 있지만 내부 스크롤과는 충돌하지 않습니다.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No breaking changes.
ScreenShots (If needed)
Light mode
Dark mode
Other information:
None
Summary by CodeRabbit
New Features
Bug Fixes
Tests