[REFACTOR/#317] 알림 권한 로직 개선 #318
Conversation
- 알림 권한 요청 로직 삭제 (TimeReminderScreen) - Home 화면 진입 시 알림 권한 확인하도록 수정 (HomeScreen)
WalkthroughRemoved POST_NOTIFICATIONS runtime flow from TimeReminder, added Android 13+ permission check in Home and wired results to HomeViewModel.sendNotification; refactored HomeViewModel to suspend-based concurrent loaders; changed inspection time APIs/timezone handling and added localization helpers; bumped app version and updated related UI strings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeScreen
participant AndroidOS as Android OS
participant HomeViewModel
rect #f2f8ff
HomeScreen->>AndroidOS: Check POST_NOTIFICATIONS (API 33+)
alt Not granted
HomeScreen->>AndroidOS: Request permission
AndroidOS-->>HomeScreen: onActivityResult(isGranted)
else Granted or <33
HomeScreen-->>HomeScreen: treat as granted
end
HomeScreen->>HomeViewModel: sendNotification(isGranted)
HomeViewModel->>HomeViewModel: build notification payload (uses token/info)
HomeViewModel->>AndroidOS: dispatch notification / call remote API
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🧹 Nitpick comments (2)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (2)
93-101: Remove unused local permission state.
isNotificationPermissionGrantedis never read. Keep only the launcher and send the result to the ViewModel.- // 알림 권한 관련 상태 - val isNotificationPermissionGranted = remember { mutableStateOf(false) } - - val requestPermissionLauncher = rememberLauncherForActivityResult( + val requestPermissionLauncher = rememberLauncherForActivityResult( contract = ActivityResultContracts.RequestPermission(), ) { isGranted: Boolean -> - isNotificationPermissionGranted.value = isGranted homeViewModel.updateNotificationPermissionGranted(isGranted) }
112-127: Simplify post-check branch and consider denial UX (rationale/Settings).
- Since the local state is unused, drop writes to it and keep only the ViewModel update.
- Optional: handle denial via rationale and/or deep-link to app notification settings to avoid repeated prompts and improve UX.
- if (ContextCompat.checkSelfPermission(context, notificationPermission) != PackageManager.PERMISSION_GRANTED) { + if (ContextCompat.checkSelfPermission(context, notificationPermission) + != PackageManager.PERMISSION_GRANTED + ) { requestPermissionLauncher.launch(notificationPermission) } else { - isNotificationPermissionGranted.value = true homeViewModel.updateNotificationPermissionGranted(true) } } else { - isNotificationPermissionGranted.value = true homeViewModel.updateNotificationPermissionGranted(true) }Optional (outside this hunk): when denied, detect
ActivityCompat.shouldShowRequestPermissionRationale(...) == falseand offer a button to open:
Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply { putExtra(Settings.EXTRA_APP_PACKAGE, context.packageName) }Do you want me to add a small rationale/settings composable for the denial path?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(2 hunks)
| viewModel.sendNotification(context, true) | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t hardcode notification permission as granted; compute or consume the real state.
Passing true forces the VM down a “permission granted” path even when the user may have denied POST_NOTIFICATIONS (Android 13+). That can lead to inconsistent behavior/UX.
Refactor to drop the boolean and let the VM check permission internally:
Apply at call sites:
- viewModel.sendNotification(context, true)
+ viewModel.sendNotification(context)Outside this file (for TimeReminderViewModel), centralize the check:
fun sendNotification(context: Context) {
val hasPermission = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
ContextCompat.checkSelfPermission(
context, Manifest.permission.POST_NOTIFICATIONS
) == PackageManager.PERMISSION_GRANTED
} else {
androidx.core.app.NotificationManagerCompat.from(context).areNotificationsEnabled()
}
if (!hasPermission) {
// Handle gracefully: surface UX or enqueue a server-side schedule without local post
return
}
// Existing send logic...
}Is HomeRoute guaranteed to run (and permission be granted) before entering this onboarding screen in all flows? If not, this hardcoded true is unsafe.
Also applies to: 86-87
I’d like to inspect how sendNotification is defined and used in the ViewModel to confirm what that boolean does. Let’s grab its declaration and nearby logic:
🏁 Script executed:
#!/bin/bash
# Locate the ViewModel file containing sendNotification
model_file=$(rg -l "class TimeReminderViewModel" -g "*.kt")
echo "Model file: $model_file"
# Show all sendNotification occurrences in that file
rg -n "sendNotification" "$model_file"
# Display the function definition and its body
start_line=$(rg -n "fun sendNotification" "$model_file" | head -n1 | cut -d: -f1)
if [ -n "$start_line" ]; then
sed -n "$((start_line-5)),$((start_line+30))p" "$model_file"
else
echo "No sendNotification function found in $model_file"
fiLength of output: 2530
Let’s verify where and how the POST_NOTIFICATIONS permission is handled (and whether HomeRoute covers it before this screen):
🏁 Script executed:
#!/bin/bash
# Search for direct POST_NOTIFICATIONS references
echo "=== POST_NOTIFICATIONS usages ==="
rg -n "POST_NOTIFICATIONS" -g "*.kt"
# Search for any runtime permission request calls
echo -e "\n=== requestPermission usages ==="
rg -n "requestPermission" -g "*.kt"
# Locate HomeRoute definition/usage
echo -e "\n=== HomeRoute occurrences ==="
rg -n "HomeRoute" -g "*.kt"Length of output: 1140
Let’s verify how and where TimeReminderScreen is entered to confirm whether notification permission is always requested beforehand:
🏁 Script executed:
#!/bin/bash
# Find usages of TimeReminderScreen or its route
echo "=== TimeReminderScreen entries ==="
rg -n "TimeReminderScreen" -g "*.kt"
# Search for 'TimeReminder' across navigation files
echo -e "\n=== Navigation mentioning TimeReminder ==="
rg -n "TimeReminder" -g "app/src/main/java/com/sopt/clody/presentation/ui/**/navigation/*.kt"Length of output: 786
Ensure notification permission isn’t hard-coded as granted
The Screen calls
viewModel.sendNotification(context, true)but permission is only requested in HomeScreen—there’s no guarantee the user has granted POST_NOTIFICATIONS before reaching this onboarding screen. Hard-coding true can result in incorrect request payloads and inconsistent UX.
• In TimeReminderScreen (around lines 79–80 and 86–87), change calls from
- viewModel.sendNotification(context, true)
+ viewModel.sendNotification(context)• In TimeReminderViewModel, remove the isPermissionGranted parameter and compute permission at runtime:
fun sendNotification(context: Context) {
val hasPermission = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) ==
PackageManager.PERMISSION_GRANTED
} else {
NotificationManagerCompat.from(context).areNotificationsEnabled()
}
if (!hasPermission) {
// Prompt user or handle gracefully
return
}
// existing send logic…
}This centralizes permission logic and prevents false “granted” assumptions.
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt
around lines 79–80 and 86–87, the screen currently calls
viewModel.sendNotification(context, true), hard-coding permission as granted;
update the screen to call viewModel.sendNotification(context) (remove the
boolean) and stop assuming POST_NOTIFICATIONS is granted. In
TimeReminderViewModel, remove the isPermissionGranted parameter from
sendNotification and compute permission at runtime using SDK/compat checks
(check POST_NOTIFICATIONS on TIRAMISU+ and
NotificationManagerCompat.areNotificationsEnabled() otherwise); if permission is
not granted, handle it gracefully (prompt user or return) before proceeding with
the existing send logic.
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
- 홈 화면 진입 시 캘린더 데이터와 일일 일기 데이터를 병렬로 로드하도록 수정 - 데이터 로딩 시 Mutex를 사용하여 동시성 문제 방지 - 중복 데이터 로딩 방지 로직 추가 - `updateSelectedDiaryDate` 함수 제거 및 관련 로직 `updateYearMonthAndLoadData` 함수로 통합 - 네트워크 요청 시 `withContext(Dispatchers.IO)` 사용하여 백그라운드 스레드에서 실행되도록 수정
There was a problem hiding this comment.
TimeReminderScreen에서는 권한설정을 받아서 isPermissionGranted 값을 바꿔주면 나중에 확인 버튼을 눌렀을 때 알림 정보를 서버로 수신했지만,
홈화면에서는 현재 알림 정보를 서버로 수신하는 로직이 없어서 그걸 추가해야할 것 같습니다. 현재 _isNotificationPermissionGranted 변수 값만 바꿔주고 서버로 보내질 않고 있어서 유저가 알림 권한 거절을 해도 인앱에서 알림 여부가 안꺼질 것 같아요
- 거절하면 홈화면 들어갈때마다 요청하겠네요 ㅜ 수정 필요!
[REFACTOR/#319] 피커로 연월 변경 시 포커스 개선 및 홈 안정성 증가 시키기
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt (1)
79-79: Simplified notification calls align with centralized permission handling.The removal of the boolean parameter correctly delegates permission management to the ViewModel and aligns with the PR's objective to centralize permission logic in HomeScreen.
Also applies to: 86-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/build.gradle.kts(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(8 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/build.gradle.kts
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (2)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (1)
getNotificationInfo(62-94)app/src/main/java/com/sopt/clody/data/repositoryimpl/NotificationRepositoryImpl.kt (1)
getNotificationInfo(14-17)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/component/FailureScreen.kt (1)
FailureScreen(27-69)
🔇 Additional comments (12)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt (1)
36-36: LGTM! Permission parameter correctly removed from signature.The function signature simplification aligns with the PR objective to centralize notification permission handling in HomeScreen. This removes the responsibility of permission state management from the TimeReminder flow.
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (5)
21-31: Excellent concurrency improvements with proper imports.The addition of coroutine utilities (async, awaitAll, coroutineScope) and synchronization primitives (Mutex, withLock) shows thoughtful architecture for handling concurrent data loading operations.
128-147: Well-structured suspend function with proper error handling.The conversion to a private suspend function with proper context switching to
Dispatchers.IOfor network operations is excellent. The error handling properly updates the error state and provides meaningful feedback.
149-171: Excellent data loading architecture with comprehensive state management.The suspend function properly handles network availability, context switching, and updates multiple state flows with the response data. The error handling is consistent with the calendar loading function.
195-220: Robust concurrency control with mutex and optimized loading logic.The implementation uses a mutex to prevent race conditions during data loading and includes smart caching logic to avoid redundant network requests. The parallel loading of calendar and daily data using
coroutineScopeandawaitAllis efficient.
222-246: Excellent overloaded function for date-specific loading.This overload handles the three-parameter case (year, month, day) with similar optimization logic and concurrency safety. The parallel loading strategy is consistent and efficient.
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (6)
3-9: Appropriate imports for permission handling.The addition of permission-related imports (Manifest, Build, PackageManager, ContextCompat) and activity result APIs properly supports the runtime permission flow implementation.
90-94: Clean permission launcher implementation.The permission launcher correctly uses the modern ActivityResultContracts API and properly forwards the permission result to the ViewModel's
sendNotificationmethod.
105-117: Comprehensive permission handling for Android 13+.The implementation correctly:
- Checks for Android 13+ (TIRAMISU) to determine if POST_NOTIFICATIONS permission is needed
- Uses proper permission checking with ContextCompat
- Requests permission only when not granted
- Provides fallback for older Android versions
- Forwards the permission state to the ViewModel
This properly centralizes notification permission logic in the Home screen as intended by the PR.
119-125: Centralized data loading with proper initialization.The
LaunchedEffectcorrectly triggers the initial data loading using the new centralizedupdateYearMonthAndLoadDatamethod, ensuring both calendar and daily data are loaded on first entry.
131-136: Consistent usage of centralized loading method.All error recovery and picker selection scenarios consistently use
updateYearMonthAndLoadData, which maintains data consistency and follows the new architecture pattern.Also applies to: 334-335, 445-445
286-291: Improved date handling in diary deletion.Using
selectedDate.valuedirectly and accessing its properties (d.year,d.monthValue,d.dayOfMonth) is cleaner and more consistent than the previous approach.
| val requestDto = SendNotificationRequestDto( | ||
| isDiaryAlarm = isPermissionGranted, | ||
| isDiaryAlarm = true, | ||
| isDraftAlarm = false, | ||
| isReplyAlarm = isPermissionGranted, | ||
| isReplyAlarm = true, | ||
| time = selectedTime, | ||
| fcmToken = fcmToken, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded notification alarm settings may not reflect user preferences.
Setting isDiaryAlarm = true and isReplyAlarm = true unconditionally ignores the user's actual notification preferences. This could send unwanted notifications or override user settings.
Consider retrieving the current notification preferences before building the request:
- val requestDto = SendNotificationRequestDto(
- isDiaryAlarm = true,
- isDraftAlarm = false,
- isReplyAlarm = true,
- time = selectedTime,
- fcmToken = fcmToken,
- )
+ val notificationInfo = notificationRepository.getNotificationInfo().getOrNull()
+ val requestDto = SendNotificationRequestDto(
+ isDiaryAlarm = notificationInfo?.isDiaryAlarm ?: true,
+ isDraftAlarm = false,
+ isReplyAlarm = notificationInfo?.isReplyAlarm ?: true,
+ time = selectedTime,
+ fcmToken = fcmToken,
+ )📝 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.
| val requestDto = SendNotificationRequestDto( | |
| isDiaryAlarm = isPermissionGranted, | |
| isDiaryAlarm = true, | |
| isDraftAlarm = false, | |
| isReplyAlarm = isPermissionGranted, | |
| isReplyAlarm = true, | |
| time = selectedTime, | |
| fcmToken = fcmToken, | |
| ) | |
| val notificationInfo = notificationRepository.getNotificationInfo().getOrNull() | |
| val requestDto = SendNotificationRequestDto( | |
| isDiaryAlarm = notificationInfo?.isDiaryAlarm ?: true, | |
| isDraftAlarm = false, | |
| isReplyAlarm = notificationInfo?.isReplyAlarm ?: true, | |
| time = selectedTime, | |
| fcmToken = fcmToken, | |
| ) |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt
around lines 49 to 55, the request DTO hardcodes isDiaryAlarm and isReplyAlarm
to true which ignores user preferences; replace the hardcoded values by fetching
the user's current notification preferences (via the existing preferences
repository/use-case or a new injected provider), read the diary/reply alarm
flags (with sensible fallbacks), and use those boolean values when constructing
SendNotificationRequestDto so the request reflects the user’s actual settings.
| fun sendNotification(isGranted: Boolean) { | ||
| viewModelScope.launch { | ||
| val fcmToken = fcmTokenProvider.getToken().orEmpty() | ||
| val notificationInfo = getNotificationInfo() ?: return@launch | ||
| val requestDto = SendNotificationRequestDto( | ||
| isDiaryAlarm = isGranted, | ||
| isDraftAlarm = notificationInfo.isDraftAlarm, | ||
| isReplyAlarm = isGranted, | ||
| time = notificationInfo.time, | ||
| fcmToken = fcmToken, | ||
| ) | ||
| notificationRepository.sendNotification(requestDto) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify notification permission handling logic.
The sendNotification function updates alarm settings based on the isGranted parameter, but there's potential inconsistency in how permissions are handled:
- Line 370:
isDiaryAlarm = isGranted- This sets diary alarms based on permission - Line 372:
isReplyAlarm = isGranted- This sets reply alarms based on permission - Line 371:
isDraftAlarm = notificationInfo.isDraftAlarm- This preserves existing draft alarm setting
This mixed approach might be confusing. Consider whether all notification types should respect the permission state or if draft alarms should also be affected.
Also, the function doesn't handle the response or potential failures from sendNotification(). Consider adding error handling:
fun sendNotification(isGranted: Boolean) {
viewModelScope.launch {
val fcmToken = fcmTokenProvider.getToken().orEmpty()
val notificationInfo = getNotificationInfo() ?: return@launch
val requestDto = SendNotificationRequestDto(
isDiaryAlarm = isGranted,
isDraftAlarm = notificationInfo.isDraftAlarm,
isReplyAlarm = isGranted,
time = notificationInfo.time,
fcmToken = fcmToken,
)
- notificationRepository.sendNotification(requestDto)
+ notificationRepository.sendNotification(requestDto).fold(
+ onSuccess = { /* Handle success if needed */ },
+ onFailure = { error ->
+ setErrorState(true, errorMessageProvider.getTemporaryError())
+ }
+ )
}
}🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository root: $(pwd)"
echo "Searching for SendNotificationRequestDto..."
rg --hidden --no-ignore-vcs --line-number "SendNotificationRequestDto" || true
echo
echo "Searching for notificationRepository.sendNotification usages..."
rg --hidden --no-ignore-vcs --line-number "notificationRepository.sendNotification" || true
echo
echo "Searching for sendNotification definitions/usages..."
rg --hidden --no-ignore-vcs --line-number "sendNotification(" || true
echo
echo "Show HomeViewModel.kt around the function (if present)..."
rg --hidden --no-ignore-vcs --line-number "class .*HomeViewModel|fun sendNotification\\(" || true
sed -n '1,240p' app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt || true
sed -n '240,520p' app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt || true
echo
echo "Search for getNotificationInfo, fcmTokenProvider, isDraftAlarm..."
rg --hidden --no-ignore-vcs --line-number "getNotificationInfo|fcmTokenProvider|isDraftAlarm" || true
echo
echo "Search for definition of SendNotificationRequestDto and NotificationRepository..."
rg --hidden --no-ignore-vcs --line-number "data class SendNotificationRequestDto|class SendNotificationRequestDto|interface NotificationRepository|class NotificationRepository" || true
rg --hidden --no-ignore-vcs --line-number "sendNotification\\s*\\(" || trueLength of output: 31519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt || true
sed -n '240,480p' app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt || trueLength of output: 9057
Fix HomeViewModel.sendNotification: align draft-alarm behaviour & add error handling
Short: I verified the code. HomeViewModel.sendNotification sets diary/reply alarms to isGranted but preserves draft alarm, and it currently ignores the Result from notificationRepository.sendNotification(...) — this is inconsistent with other VMs (NotificationSettingViewModel and HomeViewModel.sendDraftAlarmRequest) which handle each notification type explicitly and use .fold to handle success/failure.
Files to review:
- app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt
- sendNotification(isGranted: Boolean) — lines ~365-378
- sendDraftAlarmRequest(request: SendNotificationRequestDto) — earlier in same file (uses .fold)
- app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt
- changeAlarm(...) — example of per-type toggle logic + .fold handling
Suggested change (keep behaviour decision explicit and handle repository result):
fun sendNotification(isGranted: Boolean) {
viewModelScope.launch {
val fcmToken = fcmTokenProvider.getToken().orEmpty()
val notificationInfo = getNotificationInfo() ?: return@launch
val requestDto = SendNotificationRequestDto(
isDiaryAlarm = isGranted,
- isDraftAlarm = notificationInfo.isDraftAlarm,
+ // Decide behaviour: either preserve draft alarm (as today) or align with permission.
+ // isDraftAlarm = notificationInfo.isDraftAlarm,
+ isDraftAlarm = isGranted, // OR keep notificationInfo.isDraftAlarm if draft should be independent
isReplyAlarm = isGranted,
time = notificationInfo.time,
fcmToken = fcmToken,
)
- notificationRepository.sendNotification(requestDto)
+ val result = notificationRepository.sendNotification(requestDto)
+ result.fold(
+ onSuccess = { /* refresh local info if needed */ },
+ onFailure = { setErrorState(true, errorMessageProvider.getTemporaryError()) }
+ )
+ if (result.isSuccess) {
+ // refresh server state locally (suspend allowed here)
+ getNotificationInfo()
+ }
}
}Action required:
- Decide and document desired behaviour for isDraftAlarm when overall notification permission changes (preserve vs. set to isGranted).
- Implement chosen behaviour and add Result handling as shown (mirror existing .fold usage elsewhere).
- Ensure UI state is refreshed on success (e.g., call getNotificationInfo()) and errors surface via setErrorState or a suitable state.
📝 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.
| fun sendNotification(isGranted: Boolean) { | |
| viewModelScope.launch { | |
| val fcmToken = fcmTokenProvider.getToken().orEmpty() | |
| val notificationInfo = getNotificationInfo() ?: return@launch | |
| val requestDto = SendNotificationRequestDto( | |
| isDiaryAlarm = isGranted, | |
| isDraftAlarm = notificationInfo.isDraftAlarm, | |
| isReplyAlarm = isGranted, | |
| time = notificationInfo.time, | |
| fcmToken = fcmToken, | |
| ) | |
| notificationRepository.sendNotification(requestDto) | |
| } | |
| } | |
| fun sendNotification(isGranted: Boolean) { | |
| viewModelScope.launch { | |
| val fcmToken = fcmTokenProvider.getToken().orEmpty() | |
| val notificationInfo = getNotificationInfo() ?: return@launch | |
| val requestDto = SendNotificationRequestDto( | |
| isDiaryAlarm = isGranted, | |
| // Decide behaviour: either preserve draft alarm (as today) or align with permission. | |
| // isDraftAlarm = notificationInfo.isDraftAlarm, | |
| isDraftAlarm = isGranted, // OR keep notificationInfo.isDraftAlarm if draft should be independent | |
| isReplyAlarm = isGranted, | |
| time = notificationInfo.time, | |
| fcmToken = fcmToken, | |
| ) | |
| val result = notificationRepository.sendNotification(requestDto) | |
| result.fold( | |
| onSuccess = { /* refresh local info if needed */ }, | |
| onFailure = { setErrorState(true, errorMessageProvider.getTemporaryError()) } | |
| ) | |
| if (result.isSuccess) { | |
| // refresh server state locally (suspend allowed here) | |
| getNotificationInfo() | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt (4)
39-41: Confirm TimePeriod.getLabel() visibility (extension vs. member) and importsIf
getLabel()is an extension defined undercom.sopt.clody.presentation.utils.extension, it won’t be in scope with only the enum import. You need to import the extension explicitly; otherwise, this will not compile. If it’s a member on the enum, you can ignore this.Suggested import (only if
getLabelis an extension):@@ import com.sopt.clody.ui.theme.ClodyTheme +import com.sopt.clody.presentation.utils.extension.getLabelAlso, good call keeping the label derivation un-memoized so locale changes recompose the labels correctly.
45-53: Avoid dual sources of truth: selectedItem vs. startIndexYou initialize picker state via
selectedItemand also passstartIndexto eachClodyPicker. If these ever drift, the UI can momentarily jump or select inconsistently. Prefer one source of truth. IfClodyPickerhonorsstate.selectedItem, drop the explicitstartIndex. IfstartIndexis required, derive it from the state to keep them in sync.Example aligning startIndex to the state-selected item (apply analogous changes to hour/minute):
@@ ClodyPicker( state = amPmPickerState, - items = amPmLabelItems, - startIndex = 1, + items = amPmLabelItems, + startIndex = amPmLabelItems.indexOf(amPmPickerState.selectedItem).coerceAtLeast(0), visibleItemsCount = 3, infiniteScroll = false, ... )Same idea for hour/minute:
- startIndex = 8, + startIndex = hourItems.indexOf(hourPickerState.selectedItem).coerceAtLeast(0),- startIndex = 3, + startIndex = minuteItems.indexOf(minutePickerState.selectedItem).coerceAtLeast(0),Alternatively, remove all three
startIndexarguments and rely solely on state.
148-154: Prefer direct label→enum mapping over indexOf to reduce fragilityUsing
indexOfon the label list works, but a direct map is clearer and safer if item ordering or contents change.Apply:
- val selectedLabel = amPmPickerState.selectedItem - val selectedPeriod = amPmEnumItems.getOrElse(amPmLabelItems.indexOf(selectedLabel)) { TimePeriod.PM } - val selectedHour = hourPickerState.selectedItem - val selectedMinute = minutePickerState.selectedItem - - onConfirm(selectedPeriod, selectedHour, selectedMinute) + val labelToPeriod = amPmEnumItems.associateBy { it.getLabel() } + val selectedPeriod = labelToPeriod[amPmPickerState.selectedItem] ?: TimePeriod.PM + val selectedHour = hourPickerState.selectedItem + val selectedMinute = minutePickerState.selectedItem + onConfirm(selectedPeriod, selectedHour, selectedMinute)Note: Computing the map inside the click keeps it locale-accurate without sticky remembered state.
45-53: Consider making the default selections configurableHardcoding defaults to PM/9/30 limits reuse and may ignore previously saved user preferences. Expose them as optional parameters with sensible defaults.
Example (outside the changed region; for illustration):
@Composable fun NotificationSettingTimePicker( onDismissRequest: () -> Unit, onConfirm: (TimePeriod, String, String) -> Unit, defaultPeriod: TimePeriod = TimePeriod.PM, defaultHour: String = "9", defaultMinute: String = "30", ) { val amPmEnumItems = listOf(TimePeriod.AM, TimePeriod.PM) val amPmLabelItems = amPmEnumItems.map { it.getLabel() } val amPmPickerState = rememberPickerState().apply { selectedItem = defaultPeriod.getLabel() } val hourPickerState = rememberPickerState().apply { selectedItem = defaultHour } val minutePickerState = rememberPickerState().apply { selectedItem = defaultMinute } ... }app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProvider.kt (1)
8-8: Clarify contract and prefer stronger typing for inspection-time APIThe method name suggests "text", but it actually formats raw timestamps passed in as strings. Please document the expected input format (e.g., "yyyy-MM-dd'T'HH:mm[:ss]"), timezone assumption (server = Asia/Seoul), and nullability behavior. Consider accepting ZonedDateTime/Instant (or a small value object) instead of String to avoid parse fragility.
Minimal doc addition example:
interface LanguageProvider { fun getCurrentLanguageTag(): String - fun getInspectionTimeText(start: String, end: String): String? + /** + * Formats an inspection window to a localized, timezone-aware string for UI. + * + * @param start ISO-8601 local datetime string (server time, e.g. Asia/Seoul), ex: 2025-08-11T18:00[:ss] + * @param end ISO-8601 local datetime string (server time, e.g. Asia/Seoul), ex: 2025-08-11T20:00[:ss] + * @return Localized string like "Aug 11 (Mon) 18:00 ~ 20:00", or null on parse failure. + */ + fun getInspectionTimeText(start: String, end: String): String? fun getLoginType(): OAuthProviderapp/src/main/java/com/sopt/clody/domain/appupdate/AppUpdateChecker.kt (1)
8-8: Method name/return-type mismatch: returns raw times, not "text"
getInspectionTimeText()now returnsPair<String, String>?, which are raw timestamps rather than a formatted "text". Recommend renaming and/or returning a typed value to avoid confusion and accidental misuse.Options:
- Rename for clarity:
- suspend fun getInspectionTimeText(): Pair<String, String>? + suspend fun getInspectionWindowIso(): Pair<String, String>?
- Or introduce a domain type:
data class InspectionWindow(val start: ZonedDateTime, val end: ZonedDateTime) suspend fun getInspectionWindow(): InspectionWindow?If you adopt either, update call sites (SplashViewModel and LanguageProviderImpl).
app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt (2)
67-71: Verify placeholders and localization in string resourcesPlease ensure both
values/strings.xmlandvalues-ko/strings.xmldefine:
dialog_inspection_titlewith no placeholders.dialog_inspection_descriptionwith exactly one%splaceholder forinspectionTime. Mismatches across locales can throwMissingFormatArgumentException.dialog_inspection_confirmwith no placeholders.Also consider using
<xliff:g>for translator context and to prevent placeholder reordering issues.Also applies to: 74-78, 88-91
104-104: Align preview sample with runtime formattingPreview uses a 12-hour clock ("12:00 PM") while your formatter (per PR description) appears to use 24-hour "HH:mm". Aligning these avoids confusion when designers/devs compare preview vs. runtime.
app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt (2)
68-73: Avoid double Remote Config reads; fetch window once and reuse
isUnderInspection()andgetInspectionTimeText()both read start/end from Remote Config. Consider a single call that returns the window (and you derive both the boolean and the formatted text), reducing network/IPC and potential inconsistencies.
68-82: InspectionTimeText Null-Safety Confirmed; Optional Fallback Recommended
- In SplashScreen.kt (line 99), you’re already calling
which convertsinspectionTime = state.inspectionTimeText.orEmpty()nullto""and prevents anullbeing passed intostringResource.- If showing an empty dialog caption when the time parsing fails isn’t ideal, consider supplying a user-friendly default. For example:
@Composable fun SplashScreen(...) { ... if (state.showInspectionDialog) { val displayTime = state.inspectionTimeText .takeIf(String::isNotBlank) ?: stringResource(R.string.inspection_time_unavailable) InspectionDialog( inspectionTime = displayTime, onDismiss = { … } ) } }- Alternatively, you could move this fallback into the ViewModel by setting
so thatval inspectionText = inspectionTextRaw ?.let { (start, end) -> languageProvider.getInspectionTimeText(start, end) } ?: languageProvider.getInspectionTimeUnavailableText()inspectionTimeTextis never blank in state.app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt (1)
64-66: Timezone constant duplication—centralize to avoid drift
SERVER_TIMEZONEis also defined inLanguageProviderImplper PR notes. Consider moving it to a sharedTimeConstantsin domain/common to keep server assumptions consistent.app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProviderImpl.kt (3)
20-30: Make server time parsing robust to ISO-8601 variants (offset/Z-suffixed).LocalDateTime.parse(...) only accepts ISO_LOCAL_DATE_TIME (no offset). If the backend ever returns an offset (e.g., 2025-08-26T09:00:00+09:00) or Z time (2025-08-26T00:00:00Z), this will return null via runCatching and silently drop the inspection text. Prefer a tolerant parser that handles ZonedDateTime/OffsetDateTime/Instant, and finally falls back to LocalDateTime in SERVER_TIMEZONE.
Apply this diff to harden parsing:
@@ - return runCatching { - val serverZone = ZoneId.of(SERVER_TIMEZONE) - val userZone = ZoneId.systemDefault() - - val startUser = LocalDateTime.parse(start).atZone(serverZone).withZoneSameInstant(userZone) - val endUser = LocalDateTime.parse(end).atZone(serverZone).withZoneSameInstant(userZone) - - formatInspectionTime(startUser, endUser) - }.getOrNull() + return runCatching { + val serverZone = ZoneId.of(SERVER_TIMEZONE) + val userZone = ZoneId.systemDefault() + + val startUser = parseServerDateTime(start, serverZone).withZoneSameInstant(userZone) + val endUser = parseServerDateTime(end, serverZone).withZoneSameInstant(userZone) + + formatInspectionTime(startUser, endUser) + }.getOrNull() } + + private fun parseServerDateTime(raw: String, serverZone: ZoneId): ZonedDateTime { + // Try the most specific/portable forms first; fall back to naive server-local times. + return try { + ZonedDateTime.parse(raw) + } catch (_: Throwable) { + try { + java.time.OffsetDateTime.parse(raw).toZonedDateTime() + } catch (_: Throwable) { + try { + java.time.Instant.parse(raw).atZone(serverZone) + } catch (_: Throwable) { + LocalDateTime.parse(raw).atZone(serverZone) + } + } + } + }And add required imports:
+import java.time.Instant +import java.time.OffsetDateTimeOptionally, if end < start due to a backend mistake, consider swapping or logging rather than silently returning null. I can wire a small guard if you’d like.
32-43: Tighten formatting: consistent comma usage and same-day compaction.
- EN punctuation: left side uses a comma after date, right side doesn’t. Make both consistent.
- UX: When start/end are the same day, omit the duplicated date for readability (both KO/EN).
Apply this diff to formatInspectionTime:
- private fun formatInspectionTime(startUser: ZonedDateTime, endUser: ZonedDateTime): String { - return if (isKorean()) { - val koPattern = DateTimeFormatter.ofPattern(INSPECTION_TIME_FORMAT_KO, Locale.KOREAN) - "${startUser.format(koPattern)} ~ ${endUser.format(koPattern)}" - } else { - val enDateFormatter = DateTimeFormatter.ofPattern(INSPECTION_DATE_FORMAT_EN, Locale.ENGLISH) - val enTimeFormatter = DateTimeFormatter.ofPattern(INSPECTION_TIME_FORMAT_EN, Locale.ENGLISH) - val left = "${startUser.format(enDateFormatter)}, ${startUser.format(enTimeFormatter)}" - val right = "${endUser.format(enDateFormatter)} ${endUser.format(enTimeFormatter)}" - "$left ~ $right" - } - } + private fun formatInspectionTime(startUser: ZonedDateTime, endUser: ZonedDateTime): String { + val sameDay = startUser.toLocalDate() == endUser.toLocalDate() + return if (isKorean()) { + val ko = DateTimeFormatter.ofPattern(INSPECTION_TIME_FORMAT_KO, Locale.KOREAN) + if (sameDay) { + // 8/26(화) 13시00분 ~ 15시00분 + "${startUser.format(ko)} ~ ${endUser.format(DateTimeFormatter.ofPattern(\"HH시mm분\", Locale.KOREAN))}" + } else { + "${startUser.format(ko)} ~ ${endUser.format(ko)}" + } + } else { + val enDate = DateTimeFormatter.ofPattern(INSPECTION_DATE_FORMAT_EN, Locale.ENGLISH) + val enTime = DateTimeFormatter.ofPattern(INSPECTION_TIME_FORMAT_EN, Locale.ENGLISH) + if (sameDay) { + // Aug 26 (Tue), 13:00 ~ 15:00 + "${startUser.format(enDate)}, ${startUser.format(enTime)} ~ ${endUser.format(enTime)}" + } else { + // Aug 26 (Tue), 13:00 ~ Aug 27 (Wed), 09:00 + val left = "${startUser.format(enDate)}, ${startUser.format(enTime)}" + val right = "${endUser.format(enDate)}, ${endUser.format(enTime)}" + "$left ~ $right" + } + } + }Optional: consider honoring device 12/24‑hour preference for EN instead of hardcoding HH:mm. I can provide a version using DateFormat.is24HourFormat with a small Context dependency if you want.
58-66: Visibility changes to constants: verify no external usages; consider caching formatters.
- Making LANGUAGE_KO/NICKNAME/DIARY constants private is fine if nothing outside references them. Please confirm there are no external usages.
- Minor perf/readability: you recreate DateTimeFormatter instances on each call. Not critical, but you could cache them in companion object.
If you choose to cache:
companion object { private const val LANGUAGE_KO = "ko" private const val SERVER_TIMEZONE = "Asia/Seoul" private const val INSPECTION_TIME_FORMAT_KO = "M/d(E) HH시mm분" private const val INSPECTION_DATE_FORMAT_EN = "MMM d (EEE)" private const val INSPECTION_TIME_FORMAT_EN = "HH:mm" + private val KO_DATE_TIME: DateTimeFormatter = + DateTimeFormatter.ofPattern(INSPECTION_TIME_FORMAT_KO, Locale.KOREAN) + private val EN_DATE: DateTimeFormatter = + DateTimeFormatter.ofPattern(INSPECTION_DATE_FORMAT_EN, Locale.ENGLISH) + private val EN_TIME: DateTimeFormatter = + DateTimeFormatter.ofPattern(INSPECTION_TIME_FORMAT_EN, Locale.ENGLISH) }Then reference KO_DATE_TIME/EN_DATE/EN_TIME in formatInspectionTime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.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(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/splash/SplashViewModel.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProvider.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProviderImpl.kt(3 hunks)app/src/main/res/values-ko/strings.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/src/main/res/values/strings.xml
- app/src/main/res/values-ko/strings.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/component/timepicker/PickerState.kt (1)
rememberPickerState(15-16)
⏰ 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/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt (1)
115-116: LGTM: feeding localized AM/PM labels into the pickerUsing
amPmLabelItemsdirectly avoids hardcoded string comparisons and plays nicely with localization.app/src/main/java/com/sopt/clody/presentation/ui/component/dialog/InspectionDialog.kt (1)
23-23: LGTM: Using stringResource for localization is correctImport and usage align with Compose best practices.
app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt (1)
50-51: Inclusive end boundary—confirm product requirement
nowServer in startZ..endZincludes the exact end timestamp as “under inspection”. Confirm whether the window should be [start, end) instead. If so, change tonowServer >= startZ && nowServer < endZ.app/src/main/java/com/sopt/clody/presentation/utils/language/LanguageProviderImpl.kt (1)
5-8: No core library desugaring required for java.time
Your module’sminSdkis set to 28 (app/build.gradle.kts:26), and java.time APIs are natively supported on Android API 26+ devices. No additionalcoreLibraryDesugaringordesugar_jdk_libssetup is needed.
| 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) | ||
| } | ||
| val serverZone = ZoneId.of(SERVER_TIMEZONE) | ||
|
|
||
| override fun getInspectionTimeText(): String? { | ||
| val start = remoteConfigDataSource.getInspectionStart() | ||
| val end = remoteConfigDataSource.getInspectionEnd() | ||
| if (start == null || end == null) return null | ||
| val nowServer = ZonedDateTime.now(serverZone) | ||
| val startZ = start.atZone(serverZone) | ||
| val endZ = end.atZone(serverZone) | ||
|
|
||
| val startText = formatDateTimeWithDayOfWeek(start) | ||
| val endText = formatDateTimeWithDayOfWeek(end) | ||
| return "$startText ~ $endText" | ||
| return nowServer in startZ..endZ | ||
| } |
There was a problem hiding this comment.
Remote Config is never fetched here; results may be stale on cold start
Unlike getAppUpdateState(), isUnderInspection() doesn’t call remoteConfigDataSource.fetch(). On app launch, this can yield stale/default values and skip a valid inspection window.
Apply this minimal fix:
override suspend fun isUnderInspection(): Boolean {
+ // Ensure Remote Config is up-to-date before reading values.
+ remoteConfigDataSource.fetch()
val start = remoteConfigDataSource.getInspectionStart() ?: return false
val end = remoteConfigDataSource.getInspectionEnd() ?: return false
val serverZone = ZoneId.of(SERVER_TIMEZONE)
val nowServer = ZonedDateTime.now(serverZone)
val startZ = start.atZone(serverZone)
val endZ = end.atZone(serverZone)
return nowServer in startZ..endZ
}If fetch() is expensive, consider internal caching or a shared preload in the app’s startup pipeline.
📝 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.
| 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) | |
| } | |
| val serverZone = ZoneId.of(SERVER_TIMEZONE) | |
| override fun getInspectionTimeText(): String? { | |
| val start = remoteConfigDataSource.getInspectionStart() | |
| val end = remoteConfigDataSource.getInspectionEnd() | |
| if (start == null || end == null) return null | |
| val nowServer = ZonedDateTime.now(serverZone) | |
| val startZ = start.atZone(serverZone) | |
| val endZ = end.atZone(serverZone) | |
| val startText = formatDateTimeWithDayOfWeek(start) | |
| val endText = formatDateTimeWithDayOfWeek(end) | |
| return "$startText ~ $endText" | |
| return nowServer in startZ..endZ | |
| } | |
| override suspend fun isUnderInspection(): Boolean { | |
| // Ensure Remote Config is up-to-date before reading values. | |
| remoteConfigDataSource.fetch() | |
| val start = remoteConfigDataSource.getInspectionStart() ?: return false | |
| val end = remoteConfigDataSource.getInspectionEnd() ?: return false | |
| val serverZone = ZoneId.of(SERVER_TIMEZONE) | |
| val nowServer = ZonedDateTime.now(serverZone) | |
| val startZ = start.atZone(serverZone) | |
| val endZ = end.atZone(serverZone) | |
| return nowServer in startZ..endZ | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt
around lines 41 to 51, isUnderInspection() currently reads inspection start/end
from remoteConfigDataSource without fetching, which can return stale values on
cold start; fix by invoking remoteConfigDataSource.fetch() (or a suspend-safe
cached-fetch helper) at the start of the method and handle failures gracefully
(e.g., return false if fetch fails) before reading start/end so the check uses
up-to-date config; if fetch is expensive, use an existing cached flag or a
light-weight fetch-if-stale helper to avoid redundant network calls.
| /** | ||
| * Firebase RemoteConfig 로부터 점검 시간을 가져와 반환하는 함수. | ||
| * | ||
| * @return 점검 시작 시간과 종료 시간을 "2025-08-11T18:00:00" 형식으로 반환 | ||
| * */ | ||
| override suspend fun getInspectionTimeText(): Pair<String, String>? { | ||
| val start = remoteConfigDataSource.getInspectionStart() ?: return null | ||
| val end = remoteConfigDataSource.getInspectionEnd() ?: return null | ||
| return start.toString() to end.toString() | ||
| } |
There was a problem hiding this comment.
Doc/format mismatch: toString() omits seconds; can break downstream parsing
KDoc promises "2025-08-11T18:00:00", but LocalDateTime.toString() can emit "2025-08-11T18:00" (no seconds). If LanguageProvider expects seconds, parsing will fail and the dialog will show null or crash.
Apply explicit formatting and fetch freshness:
-override suspend fun getInspectionTimeText(): Pair<String, String>? {
- val start = remoteConfigDataSource.getInspectionStart() ?: return null
- val end = remoteConfigDataSource.getInspectionEnd() ?: return null
- return start.toString() to end.toString()
-}
+override suspend fun getInspectionTimeText(): Pair<String, String>? {
+ // Ensure fresh values
+ remoteConfigDataSource.fetch()
+ val start = remoteConfigDataSource.getInspectionStart() ?: return null
+ val end = remoteConfigDataSource.getInspectionEnd() ?: return null
+ return start.format(ISO_LOCAL_SECOND) to end.format(ISO_LOCAL_SECOND)
+}Add the formatter and import (outside this hunk):
import java.time.format.DateTimeFormatter companion object {
private const val SERVER_TIMEZONE = "Asia/Seoul"
+ private val ISO_LOCAL_SECOND: DateTimeFormatter =
+ DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")
}If you prefer stronger typing, return Pair<ZonedDateTime, ZonedDateTime> (or a domain InspectionWindow) and let LanguageProvider handle formatting directly.
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/data/remote/appupdate/AppUpdateCheckerImpl.kt
around lines 53 to 62, the function uses LocalDateTime.toString() which can omit
seconds; change it to explicitly format both start and end with a
DateTimeFormatter that always includes seconds (e.g. pattern
"yyyy-MM-dd'T'HH:mm:ss"), add the required import
java.time.format.DateTimeFormatter at the top of the file, and (optional)
consider returning strongly-typed date/time objects (Pair<ZonedDateTime,
ZonedDateTime> or a domain InspectionWindow) instead of strings so
formatting/parsing is handled by the consumer.
📌 ISSUE
closed #317
📄 Work Description
✨ PR Point
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Refactor
Chores