[PC-1623] 키패드 표출 시 focuse 영역이 화면에 보이도록 스크롤 기능 고도화 #202
Conversation
📝 WalkthroughWalkthroughAdds a new Modifier extension Changes
Sequence DiagramsequenceDiagram
actor User
participant Focus as Focus System
participant Modifier as animateScrollToItemOnFocus
participant IME as WindowInsets/IME
participant List as LazyListState
User->>Focus: focus on input
Focus->>Modifier: onFocusChanged(isFocused=true)
Modifier->>IME: check ime visible
alt IME visible
IME-->>Modifier: imeVisible=true
Modifier->>Modifier: small delay
Modifier->>List: animateScrollToItem(index, offset)
List-->>User: item scrolled into view
else IME hidden
IME-->>Modifier: imeVisible=false
Modifier-->>User: no scroll
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
core/common-ui/src/main/java/com/puzzle/common/ui/Modifier.kt (1)
210-232: Consider addingindexto LaunchedEffect keys.The
indexparameter is not included in theLaunchedEffectkeys. If the index changes while the item is focused (e.g., due to list reordering), the scroll will target the stale index captured at effect launch time.Additionally, consider extracting the
200delay as a named constant for clarity.♻️ Suggested improvement
+private const val IME_ANIMATION_DELAY_MS = 200L + @OptIn(ExperimentalLayoutApi::class) fun Modifier.animateScrollToItemOnFocus( listState: LazyListState, index: Int, scrollOffsetDp: Dp = 20.dp ): Modifier = composed { val density = LocalDensity.current val imeBottom = WindowInsets.ime.asPaddingValues().calculateBottomPadding() var isFocused by remember { mutableStateOf(false) } - LaunchedEffect(imeBottom, isFocused) { + LaunchedEffect(imeBottom, isFocused, index) { if (isFocused && imeBottom > 0.dp) { - delay(200) + delay(IME_ANIMATION_DELAY_MS) listState.animateScrollToItem( index = index, scrollOffset = with(density) { -scrollOffsetDp.roundToPx() } ) } } this.onFocusChanged { isFocused = it.isFocused } }feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt (1)
158-158: Remove unused variable.The
densityvariable is no longer used after replacinganimateScrollWhenFocuswithanimateScrollToItemOnFocus. This is dead code that can be removed.🧹 Suggested cleanup
- val density = LocalDensity.current - Column(modifier = modifier) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/common-ui/src/main/java/com/puzzle/common/ui/Modifier.ktfeature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt (1)
125-135: LGTM! Integration looks correct.The
idx + 1calculation correctly accounts for the header item at LazyList index 0, ensuring the scroll targets the correct ValueTalkCard. The modifier usage and parameter passing are properly wired.Also applies to: 183-189
tgyuuAn
left a comment
tgyuuAn
left a comment
There was a problem hiding this comment.
해당 Modifier가 다른 디바이스에서도 온전히 동작하나요?
체크가 필요할 것 같아요.
추가적으로 해당 Modifier가 생겨나면 기존 Modifier는 제거해도 될까요 ?
| state = listState, | ||
| modifier = modifier, | ||
| contentPadding = PaddingValues(bottom = imeBottom) | ||
| contentPadding = PaddingValues(bottom = imeBottom + 40.dp) |
There was a problem hiding this comment.
| @OptIn(ExperimentalLayoutApi::class) | ||
| fun Modifier.animateScrollToItemOnFocus( | ||
| listState: LazyListState, | ||
| index: Int, | ||
| scrollOffsetDp: Dp = 20.dp | ||
| ): Modifier = composed { | ||
| val density = LocalDensity.current | ||
| val imeBottom = WindowInsets.ime.asPaddingValues().calculateBottomPadding() | ||
| var isFocused by remember { mutableStateOf(false) } | ||
|
|
||
| LaunchedEffect(imeBottom, isFocused) { | ||
| if (isFocused && imeBottom > 0.dp) { | ||
| delay(200) | ||
| listState.animateScrollToItem( | ||
| index = index, | ||
| scrollOffset = with(density) { -scrollOffsetDp.roundToPx() } | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| this.onFocusChanged { isFocused = it.isFocused } | ||
| } |
There was a problem hiding this comment.
이거 Composed 말고 Modfiier.Node()로 변경해주시는 게 좋을 것 같아요.
composed로 작성되면 상위 Composable에서 리컴포지션 일어나면 항상 Modifier가 재트리거 되거든요.
참고 : https://developer.android.com/develop/ui/compose/custom-modifiers?hl=ko
There was a problem hiding this comment.
해당 로직이 태블릿일 때 제대로 작성하지 않아 수정했습니다.
=> e6870f4
추가
아래 Column에서 스크롤 로직에서 Node를 적용해보려고 했지만 IME이가 Show/Hide일 때를 캐치하는 것이 어려워 유지하기로 했습니다. => 포커스가 변경 될 때는 잘 되지만 포커스는 그대로인 상태에서 IME만 Show/Hide가 되는 상황에서 스크롤이 작동하지 않는 문제가 있었습니다.
@Composable
fun Modifier.animateScrollWhenFocus(
scrollState: ScrollState,
verticalWeightPx: Int = 0
): Modifier = composed {
var scrollToPosition by remember { mutableFloatStateOf(0f) }
val scope = rememberCoroutineScope()
this
.onGloballyPositioned { coordinates ->
val itemTop = coordinates.positionInParent().y
scrollToPosition = itemTop + verticalWeightPx
}
.onFocusEvent {
if (it.isFocused) {
scope.launch {
delay(300L)
scrollState.animateScrollTo(scrollToPosition.toInt())
}
}
}
}
comst19
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt:
- Line 175: The composable contains an unused coroutine scope declaration "val
scope = rememberCoroutineScope()" in ValueTalkPage; delete this unused variable
(and remove the now-unused import of rememberCoroutineScope if it becomes
unused) or, if you intended asynchronous work, replace usage by launching
coroutines with that scope (e.g., use scope.launch { ... }) so the variable is
actually used; otherwise simply remove the declaration to clean up the
composable.
🧹 Nitpick comments (2)
feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt (1)
165-173: Extract magic numbers to named constants.The delay duration (200ms) and scroll offset (-100) are magic numbers that should be extracted to named constants for better maintainability and clarity. This would also help maintain consistency if these values need to be adjusted later.
♻️ Proposed refactor
At the top of the file, add constants:
private const val IME_SCROLL_DELAY_MS = 200L private const val SCROLL_OFFSET_PX = -100Then update the LaunchedEffect:
LaunchedEffect(isImeVisible, isFocused) { if (isImeVisible && isFocused) { - delay(200) + delay(IME_SCROLL_DELAY_MS) listState.animateScrollToItem( index = index, - scrollOffset = -100 + scrollOffset = SCROLL_OFFSET_PX ) } }feature/profile/src/main/java/com/puzzle/profile/graph/valuetalk/ValueTalkScreen.kt (1)
306-322: Extract magic numbers to named constants.Similar to ValueTalkPage.kt, the delay duration (200ms) and scroll offsets (160, -100) should be extracted to named constants for better maintainability and to make the different offsets' purposes clearer.
♻️ Proposed refactor
At the top of the file, add constants:
private const val IME_SCROLL_DELAY_MS = 200L private const val ANSWER_SCROLL_OFFSET_PX = -100 private const val AI_SUMMARY_SCROLL_OFFSET_PX = 160Then update the LaunchedEffect:
LaunchedEffect(isImeVisible, isAnswerFocused, isAiFocused) { if (!isImeVisible) return@LaunchedEffect val targetOffset = when { - isAiFocused -> 160 - isAnswerFocused -> -100 + isAiFocused -> AI_SUMMARY_SCROLL_OFFSET_PX + isAnswerFocused -> ANSWER_SCROLL_OFFSET_PX else -> null } targetOffset?.let { offset -> - delay(200) + delay(IME_SCROLL_DELAY_MS) listState.animateScrollToItem( index = index, scrollOffset = offset ) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.ktfeature/profile/src/main/java/com/puzzle/profile/graph/valuetalk/ValueTalkScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/profile/src/main/java/com/puzzle/profile/graph/valuetalk/ValueTalkScreen.kt (2)
feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt (1)
ValueTalkCard(153-214)feature/matching/src/main/java/com/puzzle/matching/graph/detail/page/ValueTalkPage.kt (1)
ValueTalkCard(159-216)
🔇 Additional comments (1)
feature/profile/src/main/java/com/puzzle/profile/graph/valuetalk/ValueTalkScreen.kt (1)
248-253: Verify if both IME padding approaches are needed.The LazyColumn applies
contentPadding = PaddingValues(bottom = imeBottom)at line 253, and there's also aSpacerwith.imePadding()at the end (lines 280-283). This appears to apply IME-aware bottom padding twice, which might result in excessive spacing when the keyboard is visible. Consider whether both approaches are necessary or if one should be removed.Also applies to: 278-284
| } | ||
| } | ||
|
|
||
| val scope = rememberCoroutineScope() |
There was a problem hiding this comment.
Remove unused variable.
The scope variable is created but never used in this composable.
🧹 Proposed fix
- val scope = rememberCoroutineScope()
-
Column(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.
| val scope = rememberCoroutineScope() |
🤖 Prompt for AI Agents
In
@feature/profile/src/main/java/com/puzzle/profile/graph/register/page/ValueTalkPage.kt
at line 175, The composable contains an unused coroutine scope declaration "val
scope = rememberCoroutineScope()" in ValueTalkPage; delete this unused variable
(and remove the now-unused import of rememberCoroutineScope if it becomes
unused) or, if you intended asynchronous work, replace usage by launching
coroutines with that scope (e.g., use scope.launch { ... }) so the variable is
actually used; otherwise simply remove the declaration to clean up the
composable.
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.