코멘트 작성 및 예외처리 기능 추가 #49
Conversation
…sk-certification-comment
|
@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: 5
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/yapp/twix/main/MainActivity.kt`:
- Around line 25-26: Move the two Window-level calls out of the Composable block
and into onCreate after enableEdgeToEdge(): remove
WindowCompat.setDecorFitsSystemWindows(window, false) and
WindowInsetsControllerCompat(window,
window.decorView).isAppearanceLightStatusBars = true from inside setContent and
place them directly in onCreate (after calling enableEdgeToEdge()) so they run
once on creation rather than on every recomposition.
- Line 26: Current code sets
WindowInsetsControllerCompat(...).isAppearanceLightStatusBars = true which
forces dark icons and breaks readability in dark theme; update MainActivity.kt
to set the status bar style dynamically: if you can use activity-compose 1.12.0+
prefer using SystemBarStyle.auto() (the modern API) to let the system adapt icon
color automatically, otherwise compute the boolean from Compose's
isSystemInDarkTheme() and set
WindowInsetsControllerCompat(...).isAppearanceLightStatusBars =
!isSystemInDarkTheme() so icons invert correctly for dark vs light theme.
In
`@core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentTextField.kt`:
- Around line 44-46: Add a brief comment explaining why CIRCLE_GAP can be
negative (clarify CIRCLE_PADDING_START, CIRCLE_SIZE and that overlapping circles
are intentional) and add an accessibility label to each interactive circle:
locate CIRCLE_PADDING_START, CIRCLE_SIZE and CIRCLE_GAP and insert the comment
describing intent; then update the CommentCircle usage (the Modifier.clickable
block) to include semantics { contentDescription =
stringResource(R.string.comment_circle_description, index + 1) } (or equivalent
localized string) so screen readers get meaningful labels for each circle.
In
`@core/design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.kt`:
- Line 23: updateComment에서 comment 텍스트를 COMMENT_COUNT로 잘라낼 때
TextFieldValue.selection을 그대로 복사하면 selection이 잘린 텍스트 길이를 초과하여 런타임 문제가 발생할 수 있으니,
updateComment 함수에서 comment.copy(...)를 만들 때 새 텍스트 길이(newLen =
comment.text.take(COMMENT_COUNT).length)를 기준으로 selection.start와 selection.end를
newLen 범위로 클램프하고(start = min(selection.start, newLen), end = min(selection.end,
newLen) 및 start <= end 보장) 그 클램프된 selection을 사용해 새로운 TextFieldValue를 반환하도록
수정하세요.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationViewModel.kt`:
- Around line 96-106: The upload() function only handles the canUpload == false
path and lacks the actual upload flow; add the missing upload branch for when
uiState.value.commentUiModel.canUpload is true (e.g., launch a coroutine in
viewModelScope to trigger your upload logic, update state via reduce to show
progress/success/failure and call the existing error handlers on failure), and
extract the hard-coded 1500 delay into a named constant (e.g.,
COMMENT_ERROR_DISPLAY_MS) used by the delay call; refer to upload(),
uiState.value.commentUiModel.canUpload, viewModelScope.launch, reduce {
showCommentError() }, reduce { hideCommentError() } and delay to locate and
implement the change.
🧹 Nitpick comments (10)
feature/task-certification/src/main/res/values/strings.xml (1)
4-4: 에러 메시지 문자열과 상수 동기화에 대한 고려가 필요합니다.현재 "5글자"가 문자열에 하드코딩되어 있고,
CommentUiModel.COMMENT_COUNT = 5와 별도로 관리됩니다. 향후 글자 수 제한이 변경되면 두 곳을 모두 수정해야 하므로 불일치 위험이 있습니다.개선 방안으로는 다음을 고려해 보세요:
- 포맷 문자열 사용:
"코멘트는 %d글자로 입력해주세요!"로 변경하고 코드에서 상수 값을 주입- 또는 현재 방식 유지 시, 상수 변경 시 문자열도 함께 수정해야 함을 주석으로 명시
현재 구현도 동작에는 문제가 없으므로, 향후 유지보수 시 참고하시면 됩니다.
core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentCircle.kt (1)
27-42: Dark Mode 지원을 위한 색상 처리를 고려해 보세요.현재
CommonColor.White가 배경색으로 하드코딩되어 있어, Dark Mode에서도 흰색 원이 표시됩니다. 디자인 의도에 따라 다를 수 있지만, Dark Mode 지원이 필요하다면 테마 기반 색상 사용을 권장합니다.-.background(color = CommonColor.White, shape = CircleShape), +.background(color = MaterialTheme.colorScheme.surface, shape = CircleShape),또한
CursorBar가Box중앙에 배치되어 텍스트와 겹칠 수 있습니다. 커서가 텍스트 다음 위치에 표시되어야 한다면, 텍스트와 커서의 정렬 조정이 필요할 수 있습니다. 현재 디자인 의도가 중앙 커서라면 무시하셔도 됩니다.core/design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.kt (1)
17-21:canUpload조건식을 단순화할 수 있습니다.현재 조건:
comment.text.isEmpty() || comment.text.isNotEmpty() && hasMaxCommentLength
hasMaxCommentLength가 true면 이미isNotEmpty()를 의미하므로, 다음과 같이 단순화할 수 있습니다:val canUpload: Boolean get() = - comment.text.isEmpty() || - comment.text.isNotEmpty() && - hasMaxCommentLength + isEmpty || hasMaxCommentLength가독성과 의도 전달이 더 명확해집니다.
core/design-system/src/main/java/com/twix/designsystem/keyboard/KeyBoardAsState.kt (2)
1-1: 파일명 컨벤션 확인이 필요합니다.현재 파일명이
KeyBoardAsState.kt로 "Board"가 분리되어 있습니다. Kotlin 파일명 컨벤션에 따르면KeyboardAsState.kt가 더 적합합니다.
23-33: 키보드 상태 감지의 성능 최적화를 고려해보세요.현재 구현은 기능적으로 동작하고
WindowInsets.ime.getBottom(density)접근 방식도 표준적입니다. 다만 한 가지 개선할 수 있는 부분이 있습니다.왜 개선이 필요한가:
rememberUpdatedState에서 읽는keyboard값이 composition 중에 계산되므로,WindowInsets.ime이 변할 때마다 (IME 애니메이션 중 매 프레임) 리컴포지션이 발생합니다. 이는 불필요한 성능 오버헤드를 유발합니다. Compose 공식 문서에서도 composition 중 WindowInsets 읽기를 피하도록 권장합니다.개선 방안:
snapshotFlow와distinctUntilChanged()를 조합하면 실제 상태 변화(Closed → Opened 또는 Opened → Closed)만 감지하고, 애니메이션 중간 프레임은 무시할 수 있습니다:`@Composable` fun keyboardAsState(): State<Keyboard> { val state = remember { mutableStateOf(Keyboard.Closed) } val density = LocalDensity.current LaunchedEffect(Unit) { snapshotFlow { WindowInsets.ime.getBottom(density) > 0 } .distinctUntilChanged() .map { if (it) Keyboard.Opened else Keyboard.Closed } .collect { state.value = it } } return state }현재 구현도 충분히 동작하지만, 리스트 스크롤이나 복잡한 UI에서 IME 애니메이션 성능이 중요하다면 이 패턴을 고려해볼 가치가 있습니다. 어떤 접근이 이 프로젝트의 성능 요구사항에 더 적합한지 토론해보시겠어요?
feature/task-certification/src/main/java/com/twix/task_certification/component/CameraPreviewBox.kt (1)
33-71: 잘 구현되었습니다! State Hoisting 패턴이 잘 적용되었네요.
CommentUiModel과 관련 콜백들을 상위 컴포넌트로부터 받아CommentBox에 전달하는 방식이 MVI 패턴의 단방향 데이터 플로우와 잘 맞습니다.다만, 한 가지 개선 제안이 있습니다:
- Line 49의
375.66.dp와 Line 54의73.83.dp는 디자인 스펙에서 가져온 값으로 보이는데, 이러한 매직 넘버들을 상수로 추출하면 유지보수성이 향상됩니다.private val CAMERA_PREVIEW_SIZE = 375.66.dp private val CAMERA_PREVIEW_CORNER_RADIUS = 73.83.dpfeature/task-certification/src/main/java/com/twix/task_certification/component/CommentErrorText.kt (1)
24-38:clip()수정자가 중복됩니다.
background(color, shape)는 이미 shape를 적용하므로, 동일한 shape로clip()을 호출하는 것은 불필요합니다.clip()은 자식 컴포넌트가 부모 경계를 넘어갈 때만 필요합니다.또한, 에러 메시지는 스크린 리더 사용자를 위해 접근성 처리를 고려해 보시는 것이 좋습니다.
♻️ 개선 제안
Box( modifier = modifier .width(224.dp) .height(56.dp) - .background(color = GrayColor.C400, shape = RoundedCornerShape(12.dp)) - .clip(RoundedCornerShape(12.dp)), + .background(color = GrayColor.C400, shape = RoundedCornerShape(12.dp)) + .semantics { error("Comment validation error") }, ) {core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentTextField.kt (1)
89-94: TODO 주석이 있습니다.
noClickableRipple로 수정하라는 TODO가 있는데, 이 작업을 이 PR에서 처리하실 건가요? 아니면 별도 이슈로 트래킹하시겠어요?이슈 생성을 도와드릴 수 있습니다. 원하시면 말씀해 주세요.
feature/task-certification/src/main/java/com/twix/task_certification/TaskCertificationScreen.kt (2)
255-305:DimmedScreen컴포저블의 가시성을 확인해 주세요.
DimmedScreen이public으로 선언되어 있는데, 이 컴포넌트가 다른 곳에서 재사용될 예정인가요? 만약TaskCertificationScreen내부에서만 사용된다면private으로 변경하는 것이 적절합니다.또한, 복잡한 오버레이 로직이 잘 구현되어 있습니다!
CompositingStrategy.Offscreen과BlendMode.Clear를 사용하여 원형 구멍을 뚫는 방식이 효과적입니다.♻️ 가시성 수정 제안
`@Composable` -fun DimmedScreen( +private fun DimmedScreen( isFocused: Boolean, textFieldRect: Rect, guideTextRect: Rect, focusManager: FocusManager, ) {
211-221: AnimatedContent 사용이 적절합니다.에러 상태와 타이틀 간의 전환을
AnimatedContent로 처리한 것이 좋습니다.추가 개선 제안:
label파라미터를 추가하면 Compose 도구에서 디버깅할 때 도움이 됩니다.AnimatedContent( targetState = uiState.showCommentError, label = "TitleErrorTransition" ) { isError ->
core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentTextField.kt
Show resolved
Hide resolved
...design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.kt
Outdated
Show resolved
Hide resolved
...e/task-certification/src/main/java/com/twix/task_certification/TaskCertificationViewModel.kt
Show resolved
Hide resolved
…sk-certification-comment
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
이 PR은 저희 이번주 작업 끝나고 다시 한번 얘기해보면 좋을 거 같아요! 지금 구현해야 할 게 너무 많아서 일단은 최소한의 작동은 하니까 머지하고 나중에 다시 보시죠..!
| comment.text.isEmpty() || | ||
| comment.text.isNotEmpty() && | ||
| hasMaxCommentLength |
There was a problem hiding this comment.
이렇게 하면 코멘트가 비어있어도 업로드가 가능할 거 같은데 요구사항이 코멘트 필수 아니었나요???
|
|
||
| @Immutable | ||
| data class CommentUiModel( | ||
| val comment: TextFieldValue = TextFieldValue(""), |
There was a problem hiding this comment.
텍스트필드에 입력되는 변수를 뷰모델이 관리하는 전체 상태 변수에 넣으면 키보드로 값을 입력할 때마다 전체 화면이 리컴포지션이 발생해요 그래서 가급적이면 텍스트 필드 변수는 로컬 상태 변수로 처리하고 콜백으로 뷰모델에 넘기는 게 좋을 거 같은데 어떻게 생각하시나요??
| comment.text.isNotEmpty() && | ||
| hasMaxCommentLength | ||
|
|
||
| fun updateComment(comment: TextFieldValue) = copy(comment = comment.copy(comment.text.take(COMMENT_COUNT))) |
There was a problem hiding this comment.
이렇게 구현하면 5글자를 넘어가도 입력은 계속 가능하고 입력할 때마다 copy가 호출돼서 불필요하게 업데이트가 발생할 거 같아요! 그래서 5글자 넘어가면 return해서 업데이트가 일어나지 않게 하는 건 어떨까요
| if (uiModel.isFocused) { | ||
| focusRequester.requestFocus() | ||
| awaitFrame() | ||
| keyboardController?.show() |
| interactionSource = interactionSource, | ||
| ) { | ||
| focusRequester.requestFocus() | ||
| onCommentChanged(uiModel.comment.copy(selection = TextRange(uiModel.comment.text.length))) |
There was a problem hiding this comment.
이 코드가 커서를 계속 뒤로 보내고 있어서 텍스트 중간을 수정하는 게 안되고 있어요! 그리고 스페이스바 길게 눌러서 커서 강제로 옮기면 커서가 사라지는 버그가 있습니다
There was a problem hiding this comment.
제가 코드를 봤을 때는 일단 텍스트 필드 포커스를 키보드 + isFocused로 관리하는 로직을 개선하면 좋을 거 같아요.
keyboardAsState랑 isFocused가 활용되는 방식을 봤을 때 구현 의도는
- 사진이 선택되면 자동으로 포커스 활성화
- 키보드가 내려가면 자동으로 포커스 해제
- 키보드 상태에 따라 DimmedScreen 렌더링
저는 이렇게 세가지로 파악했습니다! 근데 지금 포커스랑 키보드 관리하는 로직이 여기저기 흩어져있고 복잡하게 얽혀있어서 버그가 생겼을 때 디버깅이 조금 어려울 거 같아요. 제가 잠깐 테스트해봤는데 텍스트 필드 활성화하고 해제하는 과정에서 깜빡이는 현상이 종종 발생하고 있어요
일단 지금 포커스랑 키보드 관리하는 LaunchedEffect 두개는 다 없애도 괜찮을 거 같아요. 그리고 CommentUiModel에 focusRequestToken: Long 이거 추가해서 사진이 선택되는 이벤트가 발생할 때 업데이트해주고 CommentTextField에서 이 값을 감지해서 텍스트 필드 활성화하면 될 것 같습니다. 그리고 CommentTextField랑 DimmedScreen이 공유하는 로컬 상태 하나 추가해서 onFocusChanged 기반으로 둘의 싱크를 맞추는 것도 가능해요. 그리고 지금 키보드 옵션에 onDone 추가하셨던데 KeyboardActions에서 onDone 콜백으로 키보드 내려갈 때 공유 상태 제어해서 DimmedScreen이랑 텍스트 필드 조절하면 됩니다!
이슈 번호
#38
리뷰/머지 희망 기한 (선택)
2026.02.03
작업내용
결과물
xc.mp4
리뷰어에게 추가로 요구하는 사항 (선택)