인증샷 상세보기 인터렉션 구현 #76
인증샷 상세보기 인터렉션 구현 #76
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionEffect.kt`:
- Around line 154-158: The per-particle random size uses positional remember
inside particles.forEach (val randomSize = remember {
spec.sizeRangeDp.random().dp }) which causes sizes to shift when list items are
removed; wrap the remember in a key scoped to the particle instance (use
key(particle) { ... }) so each ReactionParticle gets its own stable remember
scope, or alternatively key off a stable identifier from ReactionParticle if the
instance isn't stable.
- Line 52: LaunchedEffect(targetReaction) won't re-run when the same enum
instance is selected repeatedly; update ReactionEffect to accept an additional
trigger key (e.g., triggerKey) and use LaunchedEffect(key1 = triggerKey, key2 =
targetReaction) or include the trigger in the key so the effect re-executes; in
the caller (e.g., ReactionSection) add an effectTrigger counter (increment it
each onSelectReaction) and pass it as triggerKey along with effectTarget (or
alternatively wrap targetReaction in an event wrapper) so
LaunchedEffect(targetReaction) is driven by a changing key and the particle
animation always runs.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionUiModel.kt`:
- Around line 17-23: The companion object's find(type: GoalReactionType)
currently throws IllegalArgumentException via UNKNOWN_REACTION_TYPE_MESSAGE
which can crash the UI if the server adds a new GoalReactionType; change the API
to a defensive one (either make a nullable finder like findOrNull(type:
GoalReactionType): ReactionUiModel? that returns entries.find { it.type == type
}, or provide a safe fallback by returning a default ReactionUiModel instance
instead of throwing) and update all call sites of ReactionUiModel.find to handle
the nullable result or the fallback; ensure you remove or repurpose
UNKNOWN_REACTION_TYPE_MESSAGE accordingly to reflect the new behavior.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/swipe/SwipeableCard.kt`:
- Around line 131-144: The dismiss tween animations (offsetX.animateTo,
offsetY.animateTo, opacity.animateTo) are launched asynchronously so onSwipe()
and offsetX.snapTo/offsetY.snapTo run immediately and cancel those tweens;
change this so the code waits for the dismiss animations to complete before
calling onSwipe() and snapTo. Concretely, either run animateTo in a suspending
way (no launch) or capture the Jobs returned by launch for
offsetX.animateTo/offsetY.animateTo/opacity.animateTo and wait for them (join or
use coroutineScope + joinAll) before calling onSwipe() and then snapTo with
spec.reappearOffsetRatio; reference the existing symbols offsetX.animateTo,
offsetY.animateTo, opacity.animateTo, onSwipe(), offsetX.snapTo/offsetY.snapTo,
and spec.dismissDuration/spec.reappearOffsetRatio.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt`:
- Around line 174-198: The animation doesn't retrigger because effectTarget is
the same enum instance on reselection; update the selection handler in
TaskCertificationDetail so it also increments a trigger counter (e.g.,
effectTrigger: Int) whenever a reaction is selected and set effectTarget =
ReactionUiModel.find(type) as before; then pass both targetReaction and
effectTrigger into ReactionEffect (e.g., targetReaction = effectTarget,
effectTrigger = effectTrigger) so ReactionEffect's LaunchedEffect can use
effectTrigger as part of its key to force re-running the animation when the same
reaction is reselected.
- Around line 177-192: The Column uses Arrangement.Bottom but has no sizing
modifier so it only wraps its children and Arrangement.Bottom does nothing;
update the layout so Column fills available space (e.g., give Column a
Modifier.fillMaxSize() or Modifier.fillMaxHeight() / Modifier.weight(1f) on a
parent layout) or remove Arrangement.Bottom and the extra Spacer to make intent
clear; adjust the Box/Column modifiers around Column, Box, ReactionBar and
remove the Spacer if you choose the fill approach so the ReactionBar is actually
anchored to the bottom.
🧹 Nitpick comments (5)
feature/task-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionParticle.kt (1)
6-12:data class에Animatable(mutable 상태)을 포함하면 의미 오류가 발생할 수 있습니다.
data class는equals/hashCode/copy를 자동 생성하는데,Animatable은 참조 동등성(reference equality)만 가지므로 구조적 비교가 의미 없습니다. 또한copy()를 호출하면Animatable인스턴스가 공유되어 의도치 않은 애니메이션 상태 간섭이 발생할 수 있습니다.순수 데이터 홀더가 아니라 mutable 애니메이션 상태를 담는 컨테이너이므로, 일반
class로 변경하는 것을 권장합니다.♻️ 제안: 일반 class로 변경
-data class ReactionParticle( +class ReactionParticle( val iconRes: Int, val animX: Animatable<Float, AnimationVector1D> = Animatable(0f), val animY: Animatable<Float, AnimationVector1D> = Animatable(0f), val animScale: Animatable<Float, AnimationVector1D> = Animatable(0f), val animAlpha: Animatable<Float, AnimationVector1D> = Animatable(1f), )feature/task-certification/src/main/java/com/twix/task_certification/detail/swipe/SwipeCardSpec.kt (1)
9-25:dismissDistancePx가 고정 px 값이라 화면 밀도에 따라 다르게 동작할 수 있습니다.
dismissThresholdDp는Dp단위로 밀도 독립적인 반면,dismissDistancePx는 고정1000fpx로 설정되어 있습니다. 저밀도 디바이스(ldpi/mdpi)에서는 화면 크기 대비 과도한 이동 거리가 될 수 있고, 고밀도 디바이스에서는 부족할 수 있습니다.
Dp단위로 통일하고 사용 시점에toPx()로 변환하거나, 화면 너비 기반 비율로 계산하는 것이 더 일관성 있는 접근일 수 있습니다. 다만 현재도 커스터마이징이 가능하므로, 차후 개선 사항으로 고려해주세요.feature/task-certification/src/main/java/com/twix/task_certification/detail/swipe/SwipeableCard.kt (1)
33-39: Compose 가이드라인: Preview Composable 제공을 고려해주세요.코딩 가이드라인에서 Preview Composable 제공을 권장하고 있습니다.
SwipeableCard는 재사용 가능한 컴포넌트이므로, 간단한@Preview함수를 추가하면 디자인 검증과 협업에 도움이 됩니다. As per coding guidelines, "Preview Composable이 제공되는가?"feature/task-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionBar.kt (1)
81-86: 매직 넘버4→ReactionUiModel.entries.lastIndex사용을 권장합니다.현재
index < 4로 하드코딩되어 있어, 리액션 타입이 추가/삭제될 경우 이 조건도 함께 수정해야 합니다.ReactionUiModel.entries.lastIndex를 사용하면 항목 수 변경에 자동으로 대응할 수 있어 유지보수성이 향상됩니다.♻️ 제안 코드
- if (index < 4) { + if (index < ReactionUiModel.entries.lastIndex) {feature/task-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionEffectSpec.kt (1)
6-22: px 단위 속성(travelDistanceRange,spreadX)이 화면 밀도에 따라 다르게 동작할 수 있습니다.
sizeRangeDp,sidePaddingDp,startOffsetDp는 dp 단위로 올바르게 정의되어 있지만,travelDistanceRange와spreadX는 px 단위 기본값(500..1500, 180)으로 설정되어 있습니다. 이 값들은ReactionEffect에서 밀도 변환 없이 직접 사용되므로, 고밀도(xxhdpi)와 저밀도(mdpi) 기기에서 파티클 이동 거리와 좌우 퍼짐 정도가 눈에 띄게 달라질 수 있습니다.dp 단위로 통일한 뒤 사용 시점에서 px로 변환하는 것이 디바이스 간 일관된 경험을 제공하는 데 유리합니다.
♻️ 제안: dp 단위로 통일
data class ReactionEffectSpec( val particleCount: Int = 15, val staggerDelayMax: Long = 200L, val durationRange: IntRange = 800..1200, - /** 상승 거리(px) */ - val travelDistanceRange: IntRange = 500..1500, - /** 좌우 퍼짐(px) */ - val spreadX: Int = 180, + /** 상승 거리(dp) */ + val travelDistanceDpRange: IntRange = 150..450, + /** 좌우 퍼짐(dp) */ + val spreadXDp: Int = 54, /** 아이콘 크기(dp) */ val sizeRangeDp: IntRange = 32..48, val sidePaddingDp: Int = 40, val startOffsetDp: Int = 80, )사용 시점(
ReactionEffect.kt)에서with(density) { spec.travelDistanceDpRange.random().dp.toPx() }형태로 변환하면 됩니다.
| val sidePaddingPx = with(density) { spec.sidePaddingDp.dp.toPx() } | ||
| val startOffsetPx = with(density) { spec.startOffsetDp.dp.toPx() } | ||
|
|
||
| LaunchedEffect(targetReaction) { |
There was a problem hiding this comment.
LaunchedEffect(targetReaction)의 키가 동일한 리액션 재선택 시 애니메이션을 재실행하지 않습니다.
LaunchedEffect는 키가 변경될 때만 재실행됩니다. 동일한 리액션 타입을 연속으로 선택하면 targetReaction이 같은 enum 인스턴스이므로 키가 변경되지 않아 파티클 애니메이션이 발생하지 않습니다.
호출 측에서 매번 고유한 키를 제공하거나, targetReaction을 식별 가능한 이벤트 래퍼로 감싸는 방식이 필요합니다. 예를 들어:
🐛 방안 1: 이벤트 카운터를 키로 사용
ReactionEffect의 파라미터에 트리거 카운터를 추가합니다:
`@Composable`
fun ReactionEffect(
targetReaction: ReactionUiModel?,
+ triggerKey: Any,
modifier: Modifier = Modifier,
spec: ReactionEffectSpec = ReactionEffectSpec(),
) {
if (targetReaction == null) return
// ...
- LaunchedEffect(targetReaction) {
+ LaunchedEffect(triggerKey) {호출 측(ReactionSection)에서:
var effectTrigger by remember { mutableIntStateOf(0) }
// ...
onSelectReaction = { type ->
onClickReaction(type)
effectTarget = ReactionUiModel.find(type)
effectTrigger++
}
// ...
ReactionEffect(
targetReaction = effectTarget,
triggerKey = effectTrigger,
// ...
)As per coding guidelines, "remember, LaunchedEffect 등 사이드 이펙트 API가 적절히 사용되었는가?" 확인 시 발견된 이슈입니다.
🤖 Prompt for AI Agents
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionEffect.kt`
at line 52, LaunchedEffect(targetReaction) won't re-run when the same enum
instance is selected repeatedly; update ReactionEffect to accept an additional
trigger key (e.g., triggerKey) and use LaunchedEffect(key1 = triggerKey, key2 =
targetReaction) or include the trigger in the key so the effect re-executes; in
the caller (e.g., ReactionSection) add an effectTrigger counter (increment it
each onSelectReaction) and pass it as triggerKey along with effectTarget (or
alternatively wrap targetReaction in an event wrapper) so
LaunchedEffect(targetReaction) is driven by a changing key and the particle
animation always runs.
There was a problem hiding this comment.
이건 의도한거야 ! 동일한 버튼을 누르면 발생하지 않도록 하기 위해
There was a problem hiding this comment.
@chanho0908, 이해했습니다! 제 실수였네요. 동일한 리액션 버튼을 연속으로 눌렀을 때 애니메이션이 중복 실행되지 않도록 의도적으로 설계하신 거군요.
LaunchedEffect(targetReaction)을 사용하여 같은 리액션 타입에 대해서는 키가 변경되지 않으므로 자연스럽게 중복 실행을 방지하는 깔끔한 구현입니다. 불필요한 카운터나 래퍼 없이 심플하게 해결하셨네요. 👍
혼란을 드려 죄송합니다!
✏️ Learnings added
Learnt from: chanho0908
Repo: YAPP-Github/Twix-Android PR: 76
File: feature/task-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionEffect.kt:52-52
Timestamp: 2026-02-09T14:49:08.808Z
Learning: In the Twix-Android project, when implementing reaction particle animations with `ReactionEffect`, using `LaunchedEffect(targetReaction)` is intentional to prevent duplicate animations when the same reaction button is pressed repeatedly. The key remaining unchanged for the same enum instance is the desired behavior, not a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
...sk-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionEffect.kt
Outdated
Show resolved
Hide resolved
...k-certification/src/main/java/com/twix/task_certification/detail/reaction/ReactionUiModel.kt
Show resolved
Hide resolved
...e/task-certification/src/main/java/com/twix/task_certification/detail/swipe/SwipeableCard.kt
Outdated
Show resolved
Hide resolved
...sk-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt
Show resolved
Hide resolved
...sk-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt
Show resolved
Hide resolved
1 participant
이슈 번호
#52
리뷰/머지 희망 기한 (선택)
작업내용
결과물
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)
IOS에서 구현된 인터렉션 기준으로 구현했어
현재 리액션 버튼을 연타할 경우 인터렉션이 과하게 렌더링 되는데 배포 후에 스로틀링이나 디바운싱을 적용해서 해결할 예정 !