Conversation
|
""" WalkthroughA new composable dialog component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SplashViewModel
participant AppUpdateChecker
participant SplashScreen
participant InspectionDialog
User->>SplashViewModel: InitSplash intent
SplashViewModel->>AppUpdateChecker: isUnderInspection()
alt Under inspection
SplashViewModel->>SplashScreen: Update state showInspectionDialog=true, inspectionTimeText
SplashScreen->>InspectionDialog: Show with inspectionTimeText
User->>InspectionDialog: Click "확인" button
InspectionDialog->>SplashViewModel: onDismiss callback triggers DismissInspectionDialog intent
SplashViewModel->>SplashScreen: Update state showInspectionDialog=false
SplashViewModel->>App: Trigger side effect to close app
else Not under inspection
SplashViewModel->>SplashViewModel: Continue auto-login and version check
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt (2)
78-90: Button semantics & internationalisation
- Hard-coded label – move to
strings.xml.- For accessibility, add
Modifier.semantics { contentDescription = … }if the action is critical (closing the app).- Text( - text = "확인 후 앱 종료", - color = ClodyTheme.colors.gray02, - style = ClodyTheme.typography.body3SemiBold, - ) + Text( + text = stringResource(R.string.inspection_dialog_confirm_and_exit), + color = ClodyTheme.colors.gray02, + style = ClodyTheme.typography.body3SemiBold, + )
33-36: Consider parameterising dialog content for reusabilityPassing
message: String,inspectionTime: String, andconfirmText: String(with sensible defaults) would let you reuse this dialog for future maintenance windows without touching UI code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/src/main/res/drawable/img_inspection_dialog.pngis excluded by!**/*.png
📒 Files selected for processing (1)
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt (1)
app/src/main/java/com/sopt/clody/presentation/utils/base/ClodyPreview.kt (1)
BasePreview(16-23)
⏰ 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
| Image( | ||
| painter = painterResource(id = R.drawable.img_inspection_dialog), | ||
| contentDescription = null, | ||
| ) | ||
| Spacer(modifier = Modifier.height(20.dp)) |
There was a problem hiding this comment.
Image lacks an accessibility description
Provide a meaningful contentDescription or explicitly set it to an empty string "" with a comment explaining that it’s decorative.
- Image(
- painter = painterResource(id = R.drawable.img_inspection_dialog),
- contentDescription = null,
- )
+ Image(
+ painter = painterResource(id = R.drawable.img_inspection_dialog),
+ contentDescription = stringResource(R.string.desc_inspection_image), // or "" if purely decorative
+ )📝 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.
| Image( | |
| painter = painterResource(id = R.drawable.img_inspection_dialog), | |
| contentDescription = null, | |
| ) | |
| Spacer(modifier = Modifier.height(20.dp)) | |
| Image( | |
| painter = painterResource(id = R.drawable.img_inspection_dialog), | |
| contentDescription = stringResource(R.string.desc_inspection_image), // or "" if purely decorative | |
| ) | |
| Spacer(modifier = Modifier.height(20.dp)) |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt
around lines 59 to 63, the Image composable is missing a contentDescription,
which is important for accessibility. Fix this by providing a meaningful
contentDescription if the image conveys important information, or set
contentDescription to an empty string "" with a comment explaining that the
image is purely decorative.
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt
Show resolved
Hide resolved
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt
Show resolved
Hide resolved
| text = "보다 안정적인 클로디 서비스를 위해\n시스템 점검 중이에요. 곧 다시 만나요!", | ||
| color = ClodyTheme.colors.gray03, | ||
| textAlign = TextAlign.Center, | ||
| style = ClodyTheme.typography.body3Medium, | ||
| ) | ||
| Spacer(modifier = Modifier.height(8.dp)) | ||
| Text( | ||
| text = "점검시간 : ", | ||
| color = ClodyTheme.colors.gray04, | ||
| textAlign = TextAlign.Center, | ||
| style = ClodyTheme.typography.body3Medium, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded Korean strings hinder localisation & reuse
Move the message and time label to strings.xml and fetch with stringResource().
Also consider passing the time string as a parameter so the dialog can display dynamic maintenance windows.
- text = "보다 안정적인 클로디 서비스를 위해\n시스템 점검 중이에요. 곧 다시 만나요!",
+ text = stringResource(R.string.inspection_dialog_message),
...
- text = "점검시간 : ",
+ text = stringResource(R.string.inspection_dialog_time_prefix, inspectionTime),📝 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.
| text = "보다 안정적인 클로디 서비스를 위해\n시스템 점검 중이에요. 곧 다시 만나요!", | |
| color = ClodyTheme.colors.gray03, | |
| textAlign = TextAlign.Center, | |
| style = ClodyTheme.typography.body3Medium, | |
| ) | |
| Spacer(modifier = Modifier.height(8.dp)) | |
| Text( | |
| text = "점검시간 : ", | |
| color = ClodyTheme.colors.gray04, | |
| textAlign = TextAlign.Center, | |
| style = ClodyTheme.typography.body3Medium, | |
| ) | |
| // 기존 하드코딩 문자열을 리소스로 대체 | |
| text = stringResource(R.string.inspection_dialog_message), | |
| color = ClodyTheme.colors.gray03, | |
| textAlign = TextAlign.Center, | |
| style = ClodyTheme.typography.body3Medium, | |
| ) | |
| Spacer(modifier = Modifier.height(8.dp)) | |
| Text( | |
| // 시간 프리픽스도 리소스로, dynamic 매개변수(inspectionTime) 전달 | |
| text = stringResource(R.string.inspection_dialog_time_prefix, inspectionTime), | |
| color = ClodyTheme.colors.gray04, | |
| textAlign = TextAlign.Center, | |
| style = ClodyTheme.typography.body3Medium, | |
| ) |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt
around lines 65 to 76, the Korean text strings are hard-coded, which prevents
localization and reuse. Move these strings to strings.xml resource file and
replace the hard-coded text with calls to stringResource() to fetch them.
Additionally, modify the dialog to accept the maintenance time as a parameter so
it can display dynamic maintenance windows instead of a fixed label.
- isUnderInspection(): 점검 상태인지 확인하는 함수 - getInspectionTimeText(): 점검 시간 텍스트를 반환하는 함수
- inspection_start, inspection_end 키 추가 - ISO-8601 형식 문자열을 LocalDateTime으로 파싱하는 getInspectionStart(), getInspectionEnd() 함수 구현
- Remote Config로 점검 시간(시작/종료)을 받아 isUnderInspection 여부 판단 - 점검 중이라면 중단하고 다이얼로그 노출 및 제어 로직 추가 - checkVersionAndNavigate를 suspend로 변경
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt(2 hunks)app/src/main/java/com/sopt/clody/data/remote/datasource/RemoteConfigDataSource.kt(2 hunks)app/src/main/java/com/sopt/clody/domain/appupdate/AppUpdateChecker.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashContract.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.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 (15)
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashScreen.kt (2)
28-28: LGTM: Import addition is correctly placed.The import for
InspectionDialogis properly placed and follows the existing import organization.
97-104: LGTM: Inspection dialog integration follows established patterns.The conditional rendering of the inspection dialog is well-implemented:
- Consistent with the existing update dialog pattern
- Proper null safety with
.orEmpty()for the inspection time text- Correct intent dispatching for dismissal
app/src/main/java/com/sopt/clody/domain/appupdate/AppUpdateChecker.kt (1)
7-8: LGTM: Interface extension is well-designed.The new methods are appropriately designed:
isUnderInspection()as suspend function for async operationsgetInspectionTimeText()with nullable return for optional data- Clear method names that convey their purpose
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashContract.kt (2)
11-12: LGTM: State properties are well-designed.The new state properties follow established patterns:
showInspectionDialogwith appropriate default value offalseinspectionTimeTextas nullable String for optional display data
20-20: LGTM: Intent addition is consistent.The
DismissInspectionDialogintent follows the existing pattern and naming convention used for other dialog dismissal intents.app/src/main/java/com/sopt/clody/data/remote/datasource/RemoteConfigDataSource.kt (3)
6-7: LGTM: Imports are correctly added.The imports for
LocalDateTimeandDateTimeFormatterare necessary for the new inspection time parsing functionality.
25-33: LGTM: Inspection time parsing methods are robust.The implementation demonstrates good practices:
takeIf { it.isNotBlank() }prevents parsing empty/blank stringsrunCatchingsafely handles parsing exceptionsgetOrNull()returns null on parsing failures- Methods are symmetric and follow the same pattern
38-41: LGTM: Constants and formatter are appropriately defined.The new constants follow the existing naming pattern, and the
DateTimeFormatteris correctly defined as a companion object property for efficient reuse. The pattern matches the ISO-8601 format mentioned in the PR objectives.app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt (3)
7-9: LGTM: Imports are correctly added.The imports for time handling and locale formatting are necessary for the new inspection functionality.
44-52: LGTM: Inspection time text formatting is well-implemented.The method correctly handles null cases and provides a clear formatted string for the inspection period.
54-61: LGTM: Date formatting helper is well-designed.The
formatDateTimeWithDayOfWeekmethod correctly formats the date in Korean locale as intended. The string formatting matches the expected output format mentioned in the PR objectives.app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt (4)
52-52: LGTM!The new intent case follows the existing pattern and integrates well with the state management architecture.
65-77: Well-structured inspection check implementation!The function cleanly separates concerns and follows the single responsibility principle. The boolean return pattern enables clean control flow in the caller.
85-99: Good refactoring to ensure sequential execution!Removing the internal coroutine launch and making this a suspend function ensures proper sequential execution with the inspection check. This prevents race conditions where version checking might proceed even when the app is under maintenance.
117-120: LGTM!The dismissal handler correctly updates the state and triggers app closure, which is the appropriate behavior during maintenance periods.
| override suspend fun isUnderInspection(): Boolean { | ||
| val start = remoteConfigDataSource.getInspectionStart() ?: return false | ||
| val end = remoteConfigDataSource.getInspectionEnd() ?: return false | ||
| val now = LocalDateTime.now() | ||
| return now.isAfter(start) && now.isBefore(end) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider edge cases in time comparison logic.
The current implementation uses isAfter(start) && isBefore(end), which excludes the exact start and end times. For maintenance windows, you might want to include the boundaries.
Additionally, consider timezone implications when using LocalDateTime.now() if the server and client could be in different timezones.
- return now.isAfter(start) && now.isBefore(end)
+ return (now.isEqual(start) || now.isAfter(start)) && (now.isEqual(end) || now.isBefore(end))Or more concisely:
- return now.isAfter(start) && now.isBefore(end)
+ return !now.isBefore(start) && !now.isAfter(end)Would you like me to search for more information about timezone best practices for maintenance windows?
🌐 Web query:
Best practices for handling timezones in maintenance windows for mobile apps
💡 Result:
When handling timezones in maintenance windows for mobile apps, it's essential to consider user experience, accuracy, and clarity. Here are some best practices:
-
Use UTC Internally:
- Store and manage all maintenance windows in Coordinated Universal Time (UTC) to avoid confusion and simplify calculations.
-
Convert to Local Time:
- Convert UTC times to the user's local timezone before displaying maintenance windows.
- Use the device’s current timezone setting to perform conversions.
-
Clear Communication:
- Clearly specify the timezone when displaying maintenance windows if not showing local time.
- Use relative times where applicable (e.g., "Maintenance starts in 2 hours").
-
Handle Daylight Saving Time (DST):
- Use reliable timezone libraries that account for DST changes (e.g., moment-timezone in JavaScript).
- Test maintenance windows around DST changes carefully.
-
Send Reminders:
- Provide notifications or reminders about upcoming maintenance in the local timezone.
- Allow users to set reminders if possible.
-
Maintain Consistency:
- Ensure the displayed timezone matches across all parts of the app (notifications, in-app messages, emails).
-
Consider User Location Changes:
- Detect if the user changes their timezone and update the displayed maintenance window times dynamically.
-
Provide Flexibility:
- Offer users the option to view maintenance times in UTC or another preferred timezone.
-
Test Extensively:
- Test the handling of timezones and maintenance windows on devices set to different timezones, including edge cases like the International Date Line.
Would you like suggestions on specific libraries or tools to implement these best practices in your mobile app?
Ensure inclusive time checks and normalize to UTC
Please update the inspection‐window logic in app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt to:
- Include the exact start and end times (avoid excluding boundaries).
- Use UTC internally (e.g. Instant or ZonedDateTime with ZoneOffset.UTC) to eliminate timezone drift.
- Convert to the device’s local timezone only when displaying to the user.
Locations to update:
- AppUpdateCheckerImpl.kt,
isUnderInspection()method.
Suggested diff:
- override suspend fun isUnderInspection(): Boolean {
- val start = remoteConfigDataSource.getInspectionStart() ?: return false
- val end = remoteConfigDataSource.getInspectionEnd() ?: return false
- val now = LocalDateTime.now()
- return now.isAfter(start) && now.isBefore(end)
- }
+ override suspend fun isUnderInspection(): Boolean {
+ // Treat all times as UTC Instants
+ val start = remoteConfigDataSource.getInspectionStartUtc() ?: return false
+ val end = remoteConfigDataSource.getInspectionEndUtc() ?: return false
+ val now = Instant.now()
+ // inclusive: now ≥ start && now ≤ end
+ return !now.isBefore(start) && !now.isAfter(end)
+ }If your RemoteConfig returns LocalDateTime, convert it:
val startUtc = start.atZone(ZoneId.systemDefault())
.withZoneSameInstant(ZoneOffset.UTC)
.toInstant()And when showing maintenance times in‐app, convert back:
val userLocal = Instant.parse(remoteConfigTime)
.atZone(ZoneId.systemDefault())This approach:
- Guarantees inclusive windows.
- Centralizes all comparisons in UTC.
- Avoids DST and device‐timezone inconsistencies.
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt
around lines 37 to 42, update the isUnderInspection() method to perform
inclusive time checks by using isEqual or isAfter for the start time and isEqual
or isBefore for the end time. Convert the start and end LocalDateTime values to
UTC Instants before comparison by first assigning the system default zone then
converting to ZoneOffset.UTC and to Instant. Use these UTC Instants for the time
range check against the current time in UTC. This ensures all comparisons are
timezone-agnostic and inclusive of boundary times.
| private suspend fun handleInitSplash(intent: SplashContract.SplashIntent.InitSplash) { | ||
| if (intent.startIntent.hasExtra("google.message_id")) { | ||
| AmplitudeUtils.trackEvent(AmplitudeConstraints.ALARM) | ||
| } | ||
| if (checkInspectionAndHandle()) return | ||
| attemptAutoLogin() | ||
| checkVersionAndNavigate() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for the inspection check.
The sequential execution pattern is well-implemented. However, if the inspection check fails (e.g., network error when fetching remote config), the app might crash or behave unexpectedly.
Consider wrapping the inspection check in a try-catch block:
private suspend fun handleInitSplash(intent: SplashContract.SplashIntent.InitSplash) {
if (intent.startIntent.hasExtra("google.message_id")) {
AmplitudeUtils.trackEvent(AmplitudeConstraints.ALARM)
}
- if (checkInspectionAndHandle()) return
+ try {
+ if (checkInspectionAndHandle()) return
+ } catch (e: Exception) {
+ // Log error and continue with normal flow if inspection check fails
+ // This ensures the app remains functional even if remote config is unavailable
+ }
attemptAutoLogin()
checkVersionAndNavigate()
}📝 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.
| private suspend fun handleInitSplash(intent: SplashContract.SplashIntent.InitSplash) { | |
| if (intent.startIntent.hasExtra("google.message_id")) { | |
| AmplitudeUtils.trackEvent(AmplitudeConstraints.ALARM) | |
| } | |
| if (checkInspectionAndHandle()) return | |
| attemptAutoLogin() | |
| checkVersionAndNavigate() | |
| } | |
| private suspend fun handleInitSplash(intent: SplashContract.SplashIntent.InitSplash) { | |
| if (intent.startIntent.hasExtra("google.message_id")) { | |
| AmplitudeUtils.trackEvent(AmplitudeConstraints.ALARM) | |
| } | |
| try { | |
| if (checkInspectionAndHandle()) return | |
| } catch (e: Exception) { | |
| // Log error and continue with normal flow if inspection check fails | |
| // This ensures the app remains functional even if remote config is unavailable | |
| } | |
| attemptAutoLogin() | |
| checkVersionAndNavigate() | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt
around lines 56 to 63, the call to checkInspectionAndHandle() lacks error
handling, which could cause crashes if it fails. Wrap the call to
checkInspectionAndHandle() in a try-catch block to catch any exceptions, handle
errors gracefully, and prevent the app from crashing. Ensure that any caught
exceptions are logged or managed appropriately before proceeding with the
subsequent calls.
MoonsuKang
changed the title
SYAAINN
left a comment
SYAAINN
left a comment
There was a problem hiding this comment.
영어 버전 관련해서 준비가 좀 필요할 것 같아 머지는 대기하겠습니다!
There was a problem hiding this comment.
[p2]
AppUpdateChecker에서 이 역할 가져가는 이유가 있을까요? 네이밍에 비해 많은 역할을 하는 것 같아 책임분리 측면에서 AppInspectionChecker로 분리하는 방법은 어떨지 건의해봅니다.
| if (checkInspectionAndHandle()) return | ||
| attemptAutoLogin() | ||
| checkVersionAndNavigate() |
There was a problem hiding this comment.
[p5]
현재 로직 상 순서는
앱 점검 시간 체크 -> 자동 로그인 체크 -> 업데이트 버전 체크
이렇게인거 같은데, 자동 로그인보다 업데이트 버전 체크가 뒤인 이유가 있나요? 업데이트 버전 체크가 먼저 되고 로그인이 가능한 버전이면 자동 로그인을 한다는 로직이 자연스러워 보이는데 궁금합니다!
versionCode 27 -> 28 versionName 1.3.0 -> 1.4.0
Fetch 주기를 10분에서 0으로 변경. 불상사 방지
2 participants
📌 ISSUE
closed #296 , #300
📄 Work Description
Clody 서비스의 안정성을 위해 점검 시간 안내 기능을 도입했습니다.
앱 실행 시점에 Remote Config 값을 조회하여 점검 시간 범위 내라면 앱 실행을 중단하고 사용자에게 점검 안내 팝업을 노출합니다.
✨ PR Point
점검 상태에 따라 flow 자체가 중단되기 때문에 동기적 실행이 필요하다고 판단했슴다.
📸 ScreenShot/Video
Summary by CodeRabbit
Summary by CodeRabbit