Skip to content

Conversation

@comst19
Copy link
Collaborator

@comst19 comst19 commented Jan 11, 2026

1. ⭐️ 변경된 내용

  • 퍼즐 조회는 USER만 하도록 수정
  • 백그라운드 작업 실패 시 기록만 하도록 수정 => 앱 진입 시 수행하는: loadTerms, loadValuePicks, loadValueTalks

2. 🖼️ 스크린샷(선택)

3. 💡 알게된 부분

4. 📌 이 부분은 꼭 봐주세요!

Summary by CodeRabbit

  • Bug Fixes

    • Fixed unauthorized access to puzzle count functionality for non-user roles
  • Refactor

    • Enhanced error logging and handling mechanisms throughout the app
    • Optimized device token update process with improved null-safety checks

✏️ Tip: You can customize this high-level summary in your review settings.

@comst19 comst19 requested review from kkh725 and tgyuuAn January 11, 2026 13:36
@comst19 comst19 self-assigned this Jan 11, 2026
@comst19 comst19 added 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥 ㅁㅅ민수 labels Jan 11, 2026
@coderabbitai
Copy link

coderabbitai bot commented Jan 11, 2026

📝 Walkthrough

Walkthrough

Error handling refactored across multiple components by introducing a new recordError method that logs without emitting events, while replacing existing error calls. Additionally, token retrieval logic in repositories was simplified, and a role-based guard clause was added to prevent unnecessary operations.

Changes

Cohort / File(s) Summary
Error handling refactoring
app/src/main/java/com/puzzle/piece/notification/NotificationService.kt, presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt
Replaced errorHelper.sendError(it) calls with errorHelper.recordError(it) across notification and view model error handlers; now uses logging-only approach instead of event emission
New error logging method
core/domain/src/main/java/com/puzzle/domain/model/error/ErrorHelper.kt
Added new public suspend function recordError(error: Throwable) that delegates to errorRepository.logError(error) without event emission
Token retrieval optimization
core/data/src/main/java/com/puzzle/data/repository/NotificationRepositoryImpl.kt
Refactored updateDeviceToken to use firstOrNull() and isNullOrBlank() instead of property access with manual null/empty checks
Role-based guard clause
feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
Added early-return guard in fetchPuzzleCount to skip processing when currentState.userRole is not USER

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

픽스 👨‍🔧

Poem

A guard stands tall, a filter refined,
Error logs now flow in a gentler mind,
Tokens fetch cleaner with calls more kind,
No events scattered, just peace we find! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: recording background task failures instead of showing errors to users, which aligns with the primary objective of the PR.
Description check ✅ Passed The description covers the required template with the main changes documented, though some optional sections (screenshots and additional notes) are left empty.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
core/domain/src/main/java/com/puzzle/domain/model/error/ErrorHelper.kt (1)

21-23: Consider adding documentation to clarify usage.

The recordError function implementation is correct—it logs errors without emitting user-facing events. However, adding KDoc would help developers understand when to use recordError vs sendError.

📝 Suggested documentation
+    /**
+     * Records an error for logging/monitoring purposes without notifying the user.
+     * Use this for background operations or non-critical failures where user notification is unnecessary.
+     * For user-facing errors, use [sendError] instead.
+     */
     suspend fun recordError(error: Throwable) {
         errorRepository.logError(error)
     }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf43d6e and 7827005.

📒 Files selected for processing (5)
  • app/src/main/java/com/puzzle/piece/notification/NotificationService.kt
  • core/data/src/main/java/com/puzzle/data/repository/NotificationRepositoryImpl.kt
  • core/domain/src/main/java/com/puzzle/domain/model/error/ErrorHelper.kt
  • feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt
  • presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T10:27:20.895Z
Learnt from: comst19
Repo: Piece-Puzzly/Piece-Android PR: 203
File: feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt:242-246
Timestamp: 2026-01-10T10:27:20.895Z
Learning: In the Piece-Android project's SettingScreen.kt and AccessRightsPage.kt: The notification toggle represents the actual notification receivable state (combining system permission and app setting), and when system permission becomes granted, the app automatically enables push notifications through onPushNotificationCheckedChange() callback - this is intentional behavior per the app's notification policy.

Applied to files:

  • core/data/src/main/java/com/puzzle/data/repository/NotificationRepositoryImpl.kt
  • app/src/main/java/com/puzzle/piece/notification/NotificationService.kt
🧬 Code graph analysis (1)
presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt (1)
core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
  • suspendRunCatching (5-13)
⏰ 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 (4)
app/src/main/java/com/puzzle/piece/notification/NotificationService.kt (1)

41-51: LGTM! Appropriate use of recordError for background token updates.

Using recordError instead of sendError is correct here. FCM token refresh is an automatic background operation, and failures should be logged for monitoring without disrupting the user experience.

feature/matching/src/main/java/com/puzzle/matching/graph/main/MatchingViewModel.kt (1)

100-109: LGTM! Role-based guard correctly prevents unnecessary operations.

The early return guard ensures fetchPuzzleCount only executes for UserRole.USER, which aligns with the PR objective. This prevents unnecessary API calls for users in PENDING, BANNED, or NONE states who don't need puzzle count information.

presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt (1)

120-136: LGTM! Appropriate error handling for background data loading.

The three background loading operations (loadTerms, loadValuePicks, loadValueTalks) now correctly use recordError to silently log failures without showing user-facing errors. This is appropriate because:

  • These are non-critical background data prefetches performed on app startup
  • Failures don't prevent the app from functioning
  • Errors are still logged for monitoring purposes

The selective use of recordError vs sendError throughout the file is correct—critical operations like version checks (line 117) and session validation (line 152) still use sendError to alert users when necessary.

core/data/src/main/java/com/puzzle/data/repository/NotificationRepositoryImpl.kt (1)

15-21: LGTM! Defensive token validation prevents unnecessary API calls.

The change from direct access to firstOrNull() with a null/blank check is a good defensive improvement. It prevents:

  • Potential exceptions if the token flow is empty
  • Unnecessary API calls when no valid access token is available

This aligns well with the overall PR theme of more robust error handling.

Copy link
Collaborator

@kkh725 kkh725 left a comment

Choose a reason for hiding this comment

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

백그라운드 작업의 범위가 어디까지인지 고민 해 볼 필요가 있는것 같아요.
고생하셨습니다~

@comst19 comst19 merged commit 59881e6 into develop Jan 12, 2026
2 checks passed
@comst19 comst19 deleted the fix/PC-1624 branch January 12, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ㅁㅅ민수 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants