Skip to content

Conversation

@parkjiminnnn
Copy link
Contributor

#️⃣ 이슈 번호

#1


🛠️ 작업 내용

  • 소식화면 ViewPager, TabLayout 마이그레이션
  • StateFlow 적용
  • NewsViewModelTest 수정
  • Spacing 정의
  • Color 정의
  • Shapes 정의

🙇🏻 중점 리뷰 요청

  • 더이상 사용하지 않는 XML 코드 이제 지울까용??
  • 시스템 상단 바 색은 MainActivity에서 적용하는게 좋을 것 같아서 그냥 둘게요.

📸 이미지 첨부 (Optional)

Screen_recording_20251128_095307.mp4

@coderabbitai
Copy link

coderabbitai bot commented Nov 28, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (3)
  • android
  • backend
  • frontend

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'tools'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/1

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@oungsi2000 oungsi2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 제이!
궁금한 점 몇 가지 부분만 코멘트 남겨드렸어요.
너무 잘해주셨는데 살짝 걸리는 점이 있어서 한 번만 RC 요청드립니다.

다음 리뷰는 바로 머지할게요 !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shape와 Spacing을 object가 아닌 data class로 하신 이유가 있으실까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy 메서드를 통해 특정 프로퍼티를 유연하게 커스텀할 수 있을 것 같아서 data class로 했습니당

Comment on lines +29 to +31
val noticeUiState by newsViewModel.noticeUiState.collectAsStateWithLifecycle()
val lostUiState by newsViewModel.lostUiState.collectAsStateWithLifecycle()
val faqUiState by newsViewModel.faqUiState.collectAsStateWithLifecycle()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금은 괜찮지만 추후 KMP로 마이그레이션 시, KMP (ViewModel, Repository 공유) 혹은 CMP (Compose View도 공유)를 선택하느냐에 따라서 expect 함수로 분리할지 같이 고민해도 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안그래도 그 생각을 했어서 다이스한테 물어봤는데 어차피 KMP 도입하면 거의 다 엎고 다시해야한다고 해서 일단 마이그레이션부터 다하는게 좋을 것 같대요.. ㅎㅎ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Color도 CompositionLocal로 넘겨주는건 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theme의 파라미터로 들어가 있어서 그냥 뒀는데 제가 놓친 부분이 있을까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗, Spacing과 shapes는 별도의 CompositionLocal로 값을 불러오고 있는데,
Color는 MaterialTheme을 거치지 않고, 직접 값에 접근하고 있는 것 같아서 남긴 코멘트였어요 !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 이건 theme가 정해지면 한번에 바꾸려고요!

Comment on lines +29 to +30
containerColor = MaterialTheme.colorScheme.background,
contentColor = FestabookColor.black,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FestabookColor가 아닌 MaterialTheme을 사용하신 이유가 궁금합니다 !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 이건 현재 저희 theme가 정의된건 아니지만 정의된다고 가정하면 background는 무조건 흰색으로 채택될 것 같다는 생각이 들어서 background만 미리 정의해서 적용했습니당.

++ 꼭 materialTheme이 아니더라도 shapes처럼 커스텀해서 사용하는 것도 좋아보이네용.

Comment on lines +45 to +54
NewsTabPage(
pageState = pageState,
noticeUiState = noticeUiState,
faqUiState = faqUiState,
lostUiState = lostUiState,
isNoticeRefreshing = isNoticeRefreshing,
isLostItemRefreshing = isLostItemRefreshing,
onNoticeRefresh = {
val oldNotices =
(noticeUiState as? NoticeUiState.Success)?.notices ?: emptyList()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metro의 장점 중 하나는 Top Level Function Injection이 가능하다는 점이 있는데요. State 이외의 파라미터는 사실 State와 ViewModel이 결정되면 자동으로 만들어질 수 있을 것 같아요 !

한 번 도전해보는건 어떠신가요? 귀찮으시다면 패스해주셔도 좋습니다 !

Copy link
Contributor Author

@parkjiminnnn parkjiminnnn Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원래 컴포저블도 자동주입을 해볼까 했는데 찾아보니까

Metro의 top-level function injection은 주로 main 함수나 standalone 앱을 위한 것이며, Inject.kt:117-127 일반적인 Composable UI에는 권장되지 않습니다.

  1. 증분 컴파일 비호환성
    Top-level 함수 주입은 JVM에서 증분 컴파일과 호환되지 않습니다. injection-types.md:327-329 이는 빌드 성능에 심각한 영향을 줄 수 있습니다.

  2. 플랫폼 제한
    Non-JVM 타겟을 지원하지 않습니다. injection-types.md:329-331 Kotlin Multiplatform 프로젝트에서 사용할 수 없습니다.

  3. IDE 지원 부족
    IDE 지원이 매우 제한적이며, 별도의 레지스트리 플래그 활성화가 필요합니다. injection-types.md:331-332 생성된 wrapper 클래스가 IDE에서 빨간색으로 표시될 수 있습니다.

기술적 배경
이 기능은 Kotlin 컴파일러 플러그인의 FIR 단계에서 top-level 선언을 생성하는 방식으로 작동합니다. injection-types.md:194-206 하지만 이 접근 방식은 현재 Kotlin 컴파일러의 한계로 인해 위와 같은 문제들이 발생합니다.

다음과 같이 나오더라구요.. 그래서 이대로 갈까하는데 어떠신가요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오우 이런 문제들이 있었군요. Non-JVM 타겟 지원이 안되는건 꽤 흠이긴 하네요..
찾아보시느라 고생하셨습니다 !

Copy link
Contributor

@oungsi2000 oungsi2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 ~~

@parkjiminnnn parkjiminnnn merged commit 5faeee7 into develop Dec 1, 2025
14 of 17 checks passed
@parkjiminnnn parkjiminnnn deleted the feat/1 branch December 1, 2025 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants