Refactor/#56 choo qa#58
Open
starshape7 wants to merge 4 commits into
Open
Conversation
WalkthroughPR는 앱의 로고 네비게이션 기능, 스터디 리스트 아이템 클릭 콜백 구조 개선, 드롭다운 컴포넌트 마이그레이션, 그리고 레이아웃 간격 조정 등을 포함합니다. ChangesApp Bar Logo Navigation
Study Item Click Navigation Refactoring
UI Polish & Dialog Refinements
Possibly Related PRs
Poem
🎯 3 (Moderate) | ⏱️ ~20분 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/board/src/main/java/com/umcspot/spot/feature/board/post/posting/PostingScreen.kt (1)
312-338:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff접근성 시맨틱 정보 추가 필요.
ExposedDropdownMenuBox에서 수동Box+clickable구현으로 마이그레이션하면서 내장된 접근성 기능이 제거되었습니다. 현재 구현에는 스크린 리더 사용자를 위한 적절한 역할(role) 및 상태(state) 정보가 없습니다.
clickablemodifier에Modifier.semantics를 추가하여 드롭다운 역할과 확장 상태를 명시해야 합니다.♿ 접근성 개선 제안
+import androidx.compose.ui.semantics.Role +import androidx.compose.ui.semantics.contentDescription +import androidx.compose.ui.semantics.semantics +import androidx.compose.ui.semantics.stateDescriptionRow( verticalAlignment = Alignment.CenterVertically, modifier = Modifier .width(screenWidthDp(85.dp)) .border(1.dp, SpotTheme.colors.G200, SpotShapes.Hard) .clip(SpotShapes.Hard) + .semantics { + contentDescription = "게시판 선택: ${selected.korean}" + stateDescription = if (expanded) "펼쳐짐" else "접힘" + role = Role.DropdownList + } .clickable { expanded = !expanded } .padding(🤖 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 `@feature/board/src/main/java/com/umcspot/spot/feature/board/post/posting/PostingScreen.kt` around lines 312 - 338, The custom dropdown Box lost accessibility semantics when replacing ExposedDropdownMenuBox; update the Modifier chain on the Row (the one using .clickable { expanded = !expanded }) to include semantics that declare the control role and expanded state (e.g., add Modifier.semantics { role = Role.Button; this[SemanticsProperties.Expanded] = expanded } or equivalent Compose semantics API) so screen readers can announce it; ensure the semantics modifier references the existing expanded boolean and resides before or combined with the clickable modifier on the Row inside PostingScreen (the Box/Row using selected and expanded).feature/mypage/src/main/java/com/umcspot/spot/mypage/cancelMemberShip/CancelMemberShipScreen.kt (1)
50-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win일회성 이벤트를 StateFlow로 처리하는 방식 수정 필요
CancelMemberShipViewModel의leaveStatus는MutableStateFlow에 그대로 저장되며(초기값""), 성공 후_leaveStatus.value = status만 하고 idle/소비 후 리셋 로직이 없습니다. 이 상태를CancelMemberShipScreen에서LaunchedEffect(status)로 다이얼로그를 띄우면 다음 문제가 생깁니다.
- 실패(MEMBER4006) 후 재시도: 같은 결과 상태가 연속으로 동일 값이면,
MutableStateFlow가 값이 동일한 업데이트를 방출하지 않아statuskey가 바뀌지 않고LaunchedEffect가 재실행되지 않아 다이얼로그가 다시 안 뜰 수 있습니다.- 화면 회전 등 구성 변경:
showFailDialog/showSuccessDialog는remember라 초기화되지만leaveStatus는 ViewModel에 보존되어, 재구성 시 현재status로LaunchedEffect가 다시 실행되어 이미 처리한 다이얼로그가 다시 노출될 수 있습니다.
leaveStatus를 one-shot 이벤트(Channel/SharedFlow) 로 바꾸거나, 다이얼로그 표시 직후 소비 후 idle 값(""등)으로 명시적으로 리셋하는 방식으로 개선하는 게 필요합니다.🤖 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 `@feature/mypage/src/main/java/com/umcspot/spot/mypage/cancelMemberShip/CancelMemberShipScreen.kt` around lines 50 - 57, The current one-shot event handling using CancelMemberShipViewModel.leaveStatus (MutableStateFlow) causes missed or repeated dialog emissions; change leaveStatus to a one-time event (e.g., MutableSharedFlow or Channel) in CancelMemberShipViewModel and emit status as an event, then collect it in CancelMemberShipScreen using a coroutine (LaunchedEffect or collectAsStateWithLifecycle for SharedFlow) to set showFailDialog/showSuccessDialog; alternatively, if you prefer to keep StateFlow, explicitly reset _leaveStatus to an idle value ("" or null) immediately after the screen handles the status inside the LaunchedEffect(status) handler so repeated identical emissions and configuration changes don’t incorrectly suppress or re-show dialogs.
🧹 Nitpick comments (2)
feature/board/src/main/java/com/umcspot/spot/feature/board/post/posting/PostingScreen.kt (1)
332-337: 💤 Low value드롭다운 상태에 따른 화살표 아이콘 변경 고려.
현재 화살표 아이콘이
expanded상태와 관계없이 항상arrow_up으로 고정되어 있습니다. 일반적인 드롭다운 UX 패턴에서는 닫힌 상태에서는 아래 화살표, 열린 상태에서는 위 화살표를 표시하거나 회전 애니메이션을 적용합니다.시각적 피드백 개선을 위해
expanded상태에 따라 아이콘을 조건부로 변경하는 것을 고려해보세요.🎨 아이콘 상태 반영 제안
Image( - painter = painterResource(R.drawable.arrow_up), + painter = painterResource( + if (expanded) R.drawable.arrow_up else R.drawable.arrow_down + ), contentDescription = "게시판 선택", colorFilter = ColorFilter.tint(SpotTheme.colors.B500), modifier = Modifier.size(screenWidthDp(14.dp)) )또는 회전 애니메이션 사용:
+import androidx.compose.animation.core.animateFloatAsState +import androidx.compose.ui.draw.rotate + + val rotation by animateFloatAsState( + targetValue = if (expanded) 180f else 0f, + label = "arrow rotation" + ) + Image( painter = painterResource(R.drawable.arrow_down), contentDescription = "게시판 선택", colorFilter = ColorFilter.tint(SpotTheme.colors.B500), - modifier = Modifier.size(screenWidthDp(14.dp)) + modifier = Modifier + .size(screenWidthDp(14.dp)) + .rotate(rotation) )🤖 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 `@feature/board/src/main/java/com/umcspot/spot/feature/board/post/posting/PostingScreen.kt` around lines 332 - 337, The arrow Image in PostingScreen.kt is always using R.drawable.arrow_up regardless of the dropdown state; update the UI to reflect the `expanded` state by either (A) selecting the drawable conditionally (use R.drawable.arrow_down when expanded == false and R.drawable.arrow_up when expanded == true) in the Image painterResource call, or (B) apply a rotation animation using animateFloatAsState and Modifier.graphicsLayer/Modifier.rotate on the same Image so it visually points down when closed and up when opened; locate the Image block that renders the arrow and change the painter/Modifier to depend on the `expanded` boolean.feature/study/src/main/java/com/umcspot/spot/study/recruiting/RecruitingStudyScreen.kt (1)
238-261: ⚡ Quick win
studies.indexOf(item)를itemsIndexed로 대체하세요.
items루프 안에서 매 아이템마다studies.indexOf(item)을 호출하면 항목 수에 대해 O(n²) 비용이 발생합니다.loadNextPage로 리스트가 계속 커지는 페이징 화면이라 항목이 많아질수록 비용이 누적되며,equals기반 조회라 동일 값 항목에서 잘못된 인덱스를 반환할 여지도 있습니다.itemsIndexed로 인덱스를 직접 받으면 더 안전하고 효율적입니다.♻️ 제안 수정
- items( - items = studies, - key = { it.id } - ) { item -> + itemsIndexed( + items = studies, + key = { _, item -> item.id } + ) { index, item -> Spacer(Modifier.padding(screenHeightDp(5.dp))) StudyListItem( item = item, modifier = Modifier .fillMaxWidth(), onClick = { onItemClick(item.id) } ) - if(studies.indexOf(item) != studies.lastIndex) { + if (index != studies.lastIndex) { Spacer(Modifier.padding(screenHeightDp(5.dp))) HorizontalDivider( modifier = Modifier .fillMaxWidth(), color = SpotTheme.colors.G300, thickness = 1.dp ) } }
itemsIndexedimport 추가가 필요합니다:+import androidx.compose.foundation.lazy.itemsIndexed🤖 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 `@feature/study/src/main/java/com/umcspot/spot/study/recruiting/RecruitingStudyScreen.kt` around lines 238 - 261, Replace the current items(...) usage that calls studies.indexOf(item) with the itemsIndexed(...) variant so you receive the item index directly (avoid O(n²) and equals-based lookup); inside the lambda use the provided index to compare against studies.lastIndex to decide when to render the trailing Spacer and HorizontalDivider; update the call site around StudyListItem (keeping onItemClick(item.id) and modifiers unchanged) and add the itemsIndexed import so the composable compiles.
🤖 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 `@feature/main/src/main/java/com/umcspot/spot/main/MainScreen.kt`:
- Line 130: The onSearchClick callback is currently a no-op (onSearchClick = {
}) so the search icon does nothing; update the MainScreen usage by implementing
the search handler: either wire it to the existing navigation action (call the
NavController navigate method or the existing navigateToSearch function if
present) or, if search is intentionally deferred, replace the empty lambda with
a clear TODO stub that logs or emits an event (e.g., onSearchRequested) so the
behavior is traceable and testable; locate the onSearchClick parameter in
MainScreen composable invocation to make the change.
---
Outside diff comments:
In
`@feature/board/src/main/java/com/umcspot/spot/feature/board/post/posting/PostingScreen.kt`:
- Around line 312-338: The custom dropdown Box lost accessibility semantics when
replacing ExposedDropdownMenuBox; update the Modifier chain on the Row (the one
using .clickable { expanded = !expanded }) to include semantics that declare the
control role and expanded state (e.g., add Modifier.semantics { role =
Role.Button; this[SemanticsProperties.Expanded] = expanded } or equivalent
Compose semantics API) so screen readers can announce it; ensure the semantics
modifier references the existing expanded boolean and resides before or combined
with the clickable modifier on the Row inside PostingScreen (the Box/Row using
selected and expanded).
In
`@feature/mypage/src/main/java/com/umcspot/spot/mypage/cancelMemberShip/CancelMemberShipScreen.kt`:
- Around line 50-57: The current one-shot event handling using
CancelMemberShipViewModel.leaveStatus (MutableStateFlow) causes missed or
repeated dialog emissions; change leaveStatus to a one-time event (e.g.,
MutableSharedFlow or Channel) in CancelMemberShipViewModel and emit status as an
event, then collect it in CancelMemberShipScreen using a coroutine
(LaunchedEffect or collectAsStateWithLifecycle for SharedFlow) to set
showFailDialog/showSuccessDialog; alternatively, if you prefer to keep
StateFlow, explicitly reset _leaveStatus to an idle value ("" or null)
immediately after the screen handles the status inside the
LaunchedEffect(status) handler so repeated identical emissions and configuration
changes don’t incorrectly suppress or re-show dialogs.
---
Nitpick comments:
In
`@feature/board/src/main/java/com/umcspot/spot/feature/board/post/posting/PostingScreen.kt`:
- Around line 332-337: The arrow Image in PostingScreen.kt is always using
R.drawable.arrow_up regardless of the dropdown state; update the UI to reflect
the `expanded` state by either (A) selecting the drawable conditionally (use
R.drawable.arrow_down when expanded == false and R.drawable.arrow_up when
expanded == true) in the Image painterResource call, or (B) apply a rotation
animation using animateFloatAsState and Modifier.graphicsLayer/Modifier.rotate
on the same Image so it visually points down when closed and up when opened;
locate the Image block that renders the arrow and change the painter/Modifier to
depend on the `expanded` boolean.
In
`@feature/study/src/main/java/com/umcspot/spot/study/recruiting/RecruitingStudyScreen.kt`:
- Around line 238-261: Replace the current items(...) usage that calls
studies.indexOf(item) with the itemsIndexed(...) variant so you receive the item
index directly (avoid O(n²) and equals-based lookup); inside the lambda use the
provided index to compare against studies.lastIndex to decide when to render the
trailing Spacer and HorizontalDivider; update the call site around StudyListItem
(keeping onItemClick(item.id) and modifiers unchanged) and add the itemsIndexed
import so the composable compiles.
🪄 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: f40bdd68-d252-4eb8-9de9-ce6bb22e9788
📒 Files selected for processing (10)
core/designsystem/src/main/java/com/umcspot/spot/designsystem/component/appBar/AppBar.ktcore/designsystem/src/main/java/com/umcspot/spot/designsystem/component/modal/ReportModal.ktfeature/board/src/main/java/com/umcspot/spot/feature/board/post/posting/PostingScreen.ktfeature/home/src/main/java/com/umcspot/spot/home/HomeScreen.ktfeature/main/src/main/java/com/umcspot/spot/main/MainNavHost.ktfeature/main/src/main/java/com/umcspot/spot/main/MainScreen.ktfeature/mypage/src/main/java/com/umcspot/spot/mypage/cancelMemberShip/CancelMemberShipScreen.ktfeature/mypage/src/main/java/com/umcspot/spot/mypage/navigation/MyPageNavigation.ktfeature/study/src/main/java/com/umcspot/spot/study/recruiting/RecruitingStudyScreen.ktfeature/study/src/main/java/com/umcspot/spot/study/recruiting/navigation/RecruitingStudyNavigation.kt
| AppBarHome( | ||
| hasAlert = hasUnreadAlert, | ||
| onSearchClick = { /* TODO */ }, | ||
| onSearchClick = { }, |
There was a problem hiding this comment.
검색 버튼이 동작하지 않는 빈 콜백입니다.
onSearchClick = { }로 인해 검색 아이콘을 눌러도 아무 동작이 없습니다. 의도된 상태(미구현 보류)인지, 아니면 검색 화면으로의 네비게이션 연결이 누락된 것인지 확인해 주세요.
검색 네비게이션 연결이 필요하다면 핸들러 구현을 도와드리거나 추적용 이슈를 열어드릴까요?
🤖 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 `@feature/main/src/main/java/com/umcspot/spot/main/MainScreen.kt` at line 130,
The onSearchClick callback is currently a no-op (onSearchClick = { }) so the
search icon does nothing; update the MainScreen usage by implementing the search
handler: either wire it to the existing navigation action (call the
NavController navigate method or the existing navigateToSearch function if
present) or, if search is intentionally deferred, replace the empty lambda with
a clear TODO stub that logs or emits an event (e.g., onSearchRequested) so the
behavior is traceable and testable; locate the onSearchClick parameter in
MainScreen composable invocation to make the change.
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.
Related issue 🛠
Work Description 📝
Screenshot 📸
Uncompleted Tasks 😅
PR Point 📌
트러블 슈팅 💥
Summary by CodeRabbit
릴리스 노트
새로운 기능
UI 개선
개선사항