[Refactor/#309] 타임존을 고려한 API 수정사항을 반영합니다.#311
Conversation
- KST(서버) <-> UST(User's Time Zone)을 변환하는 함수를 제작했습니다.
WalkthroughTime zone handling has been integrated across the API and UI layers. A new OkHttp interceptor injects the user's system time zone into HTTP requests for specific endpoints. Time conversion utilities were introduced, and notification time selection, display, and submission logic were refactored to consistently use time zone-aware conversions. Related ViewModel and composable interfaces were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (TimePicker/Selector)
participant ViewModel
participant TimeZoneUtil
participant OkHttpClient
participant TimeZoneInterceptor
participant API Server
User->>UI (TimePicker/Selector): Selects notification time (AM/PM, hour, minute)
UI (TimePicker/Selector)->>ViewModel: onConfirm(TimePeriod, hour, minute)
ViewModel->>TimeZoneUtil: convertUTZtoKST(TimePeriod, hour, minute)
TimeZoneUtil-->>ViewModel: timeString (KST)
ViewModel->>OkHttpClient: Send API request (with timeString)
OkHttpClient->>TimeZoneInterceptor: Intercept request
TimeZoneInterceptor->>OkHttpClient: Add Time-Zone header (if endpoint matches)
OkHttpClient->>API Server: Send request with Time-Zone header
API Server-->>OkHttpClient: Response
OkHttpClient-->>ViewModel: Response
ViewModel-->>UI (TimePicker/Selector): Update state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
🔇 Additional comments (4)
✨ 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. 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: 6
🧹 Nitpick comments (4)
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderScreen.kt (1)
105-105: LGTM! Consider making default time configurable.The change correctly replaces the removed
setFixedTimemethod withsetSelectedTimeusing the new time zone conversion approach. The hardcoded default time (9:30 PM) is reasonable.Consider extracting the default time values to constants for maintainability:
companion object { private val DEFAULT_TIME_PERIOD = TimePeriod.PM private const val DEFAULT_HOUR = "9" private const val DEFAULT_MINUTE = "30" } // Usage: viewModel.setSelectedTime(DEFAULT_TIME_PERIOD, DEFAULT_HOUR, DEFAULT_MINUTE)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (1)
236-245: Business logic correctly implements time zone-based diary writing rules.The implementation properly enforces the business requirement where Korean users (Asia/Seoul) can write diary entries for today or yesterday, while users in other time zones can only write for today.
Consider extracting the time zone constant for better maintainability:
+ companion object { + private const val KOREAN_TIME_ZONE = "Asia/Seoul" + } + fun canWriteDiary(): Boolean { val userTimeZone = ZoneId.systemDefault().id val today = LocalDate.now() val selected = _selectedDate.value - val isAvailableDay = if (userTimeZone == "Asia/Seoul") { + val isAvailableDay = if (userTimeZone == KOREAN_TIME_ZONE) { selected == today || selected == today.minusDays(1) } else { selected == today } return _diaryCount.value == 0 && isAvailableDay }app/src/main/java/com/sopt/clody/data/remote/util/TimeZoneInterceptor.kt (2)
8-11: Consider thread safety and error handling for timezone capture.The current implementation captures the system timezone once at instantiation, which is efficient. However, consider these potential issues:
- Timezone changes: If the user changes their system timezone while the app is running, this won't be reflected.
- Error handling:
ZoneId.systemDefault()could potentially throw exceptions in edge cases.Consider this more robust approach:
- private val userTimeZone = ZoneId.systemDefault().id + private fun getUserTimeZone(): String = try { + ZoneId.systemDefault().id + } catch (e: Exception) { + "UTC" // fallback to UTC if system timezone is unavailable + }And update the usage:
- chain.proceed(request.newBuilder().addHeader(TIME_ZONE, userTimeZone).build()) + chain.proceed(request.newBuilder().addHeader(TIME_ZONE, getUserTimeZone()).build())
23-31: Consider improving URL matching precision.The current string-based matching works but has some concerns:
- Redundant patterns:
"api/v1/diary"already matches"api/v1/diary/time"- Potential false positives with simple
contains()checks- Could match unintended URLs containing these substrings
Consider this more precise approach:
private fun shouldAddTimeZoneHeader(url: String): Boolean { val path = url.substringAfter("//").substringAfter("/") return path.startsWith("api/v1/diary") || path.startsWith("api/v1/calendar") || path.startsWith("api/v1/reply") || path.startsWith("api/v1/draft") }Or use regex for even more precision if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/src/main/java/com/sopt/clody/data/remote/util/TimeZoneInterceptor.kt(1 hunks)app/src/main/java/com/sopt/clody/di/NetworkModule.kt(2 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/HomeViewModel.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationTimeSelector.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt(1 hunks)app/src/main/res/values-ko/strings.xml(1 hunks)app/src/main/res/values/strings.xml(2 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
[warning] 55-55: String.format("%02s:%02s", kstHour, kstMinute) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🔇 Additional comments (24)
app/src/main/res/values-ko/strings.xml (1)
107-107: LGTM! Format specifier change aligns with time zone refactoring.The change from
%3$sto%3$dfor the minute component correctly supports the new time handling approach where minutes are passed as integers rather than strings, ensuring proper formatting in the Korean locale.app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (1)
285-285: LGTM! More precise time handling.Removing the fallback default and directly using
info.timemakes the time handling more precise and consistent with the new time zone-aware approach.app/src/main/res/values/strings.xml (2)
34-34: LGTM! Guide text correctly reflects new time zone-based business rules.The updated text removes the reference to writing diary entries for yesterday, which aligns with the new business logic where only Korean users can write for yesterday while others can only write for today.
107-107: LGTM! Format specifier change supports integer minute values.The change from
%3$sto%3$dfor the minute component is consistent with the Korean locale version and properly supports the new time handling approach where minutes are passed as integers.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (3)
68-70: LGTM: Formatting improvementThe multi-line parameter formatting improves readability.
73-74: LGTM: Callback signature updated for timezone handlingThe callback signature change from a single
newNotificationTimeparameter to separatetimePeriod,hour, andminuteparameters aligns well with the new timezone-aware architecture. This allows for more precise time handling and conversion.
157-157: Time format is consistent; no action needed
Verified that the ViewModel storesnotificationTimeas a KST"HH:mm"string (from the API) and thatNotificationTimeSelectorcorrectly appliesconvertKSTtoUTZ(time)for local display. The reverse conversionconvertUTZtoKST(...)inchangeNotificationTimemaintains symmetry when sending updates back to the server.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationTimeSelector.kt (2)
17-17: LGTM: Proper timezone conversion for displayThe import and usage of
convertKSTtoUTZcorrectly converts the stored KST time to the user's local timezone for display purposes. This maintains the separation between storage format (KST) and display format (user's timezone).Also applies to: 26-26
47-47: Placeholders and conversion logic verified
- The
%1$s(label),%2$d(hour), and%3$d(minute) placeholders in
app/src/main/res/values/strings.xml:107(%2$d:%3$d %1$s) andapp/src/main/res/values-ko/strings.xml:107(%1$s %2$d시 %3$d분)
match the(String, Int, Int)arguments passed viastringResource(...).convertKSTtoUTZinTimeZoneExt.ktusesZonedDateTime.withZoneSameInstant, which correctly applies DST and offset transitions. It will propagate aDateTimeParseExceptionif the input doesn’t match"HH:mm"—ensure any external time strings are pre-validated if necessary.No changes required.
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt (2)
31-31: LGTM: Improved interface designThe callback signature change from a single
Stringparameter toTimePeriod,String,Stringparameters provides better type safety and clearer intent. Using theTimePeriodenum instead of string-based AM/PM handling is a good design choice.Also applies to: 37-37
140-141: LGTM: Better separation of concernsRemoving the time format conversion logic from the UI component and passing raw selected values improves separation of concerns. The ViewModel is now responsible for time conversion, which is more appropriate.
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (2)
10-11: LGTM: Proper imports for timezone conversionThe imports for
TimePeriodandconvertUTZtoKSTsupport the new timezone-aware notification time handling.
145-145: LGTM: Updated method signature for better parameter handlingThe method signature change to accept separate
timePeriod,hour, andminuteparameters provides better type safety and aligns with the UI component changes.app/src/main/java/com/sopt/clody/di/NetworkModule.kt (3)
10-10: LGTM: Proper dependency injection setupThe import and singleton provider for
TimeZoneInterceptorfollow proper DI patterns and ensure the interceptor is available throughout the application.Also applies to: 45-47
54-54: LGTM: Interceptor properly integrated into HTTP clientThe
TimeZoneInterceptoris correctly added to the OkHttp client's interceptor chain. The parameter addition to the provider method signature maintains proper dependency injection.Also applies to: 59-59
45-47: Confirm TimeZoneInterceptor Presence and BehaviorI ran automated searches but could not locate the
TimeZoneInterceptorimplementation in the repo. Please manually verify that:
TimeZoneInterceptor.ktexists undercom.sopt.clody.data.remote.util.- It correctly identifies and intercepts diary‐related API endpoints.
- It adds the timezone header in the expected format.
- It handles edge cases (e.g., system timezone unavailable).
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt (3)
82-84: LGTM! Clean refactoring of time conversion logic.The consolidation of time formatting into the
setSelectedTimemethod using the utility function is a good improvement. The method signature is clear and the conversion to KST for notification scheduling makes sense in the Korean app context.
13-13: LGTM!The import addition supports the time zone conversion refactoring and aligns with the PR objectives.
82-84: LGTM!The method correctly uses the time zone conversion utility to convert user input to KST format before storing it. This ensures consistent time handling when sending notifications to the API.
app/src/main/java/com/sopt/clody/data/remote/util/TimeZoneInterceptor.kt (5)
36-38: LGTM! Good use of companion object for constants.The constant definition follows Kotlin best practices and makes the header name easily maintainable.
23-31: Manual verification required: ensure complete timezone‐header coverageI wasn’t able to extract any endpoint paths from the Retrofit interfaces via automation. Please manually review all of your
api/v1/*diary, calendar, reply, and draft endpoints and confirm they’re included inshouldAddTimeZoneHeader.
- Verify every new or existing endpoint that needs the timezone header is matched.
- Update the
timeZoneRequiredPathsset if any are missing.
8-11: LGTM with note on timezone caching behavior.The class structure is well-designed with proper dependency injection and efficient timezone capture. Note that the timezone is cached at instantiation time, so runtime timezone changes won't be reflected until app restart. This is likely acceptable for most use cases but consider if dynamic timezone updates are needed.
12-21: LGTM!The intercept method is well-structured with clear conditional logic and proper delegation to helper methods. The implementation correctly handles both cases for adding or skipping the timezone header.
33-38: LGTM!The header injection implementation correctly uses OkHttp's builder pattern and follows best practices with the constant header name. The implementation is clean and efficient.
| private fun proceedWithTimeZoneHeader(chain: Interceptor.Chain, request: okhttp3.Request): Response = | ||
| chain.proceed(request.newBuilder().addHeader(TIME_ZONE, userTimeZone).build()) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider privacy implications of timezone header.
Adding the user's timezone to API requests has privacy implications:
- Location inference: Timezone can be used to infer approximate user location.
- User tracking: Combined with other data, timezone can contribute to user fingerprinting.
Ensure this aligns with your privacy policy and consider:
- Whether this data is necessary for all matched endpoints
- If the timezone data is properly handled and not logged unnecessarily on the server side
- Whether users should be informed about timezone data collection
🌐 Web query:
What are the privacy implications of sending user timezone in HTTP headers for mobile applications?
💡 Result:
Sending user timezone in HTTP headers for mobile applications has several privacy implications to consider:
-
User Profiling: Timezone information can be combined with other data (such as IP addresses, device IDs, or behavioral data) to build more comprehensive user profiles. Knowing a user's timezone narrows down their approximate geographic location, which can be used to infer habits, routines, or preferences.
-
Location Tracking: While timezone is less precise than GPS location, it still reveals approximate geographic information. This can be a privacy concern especially in regions where timezone boundaries are closely packed, allowing for more precise location inference.
-
Cross-site or Cross-service Tracking: If multiple services or domains receive timezone information consistently via HTTP headers, they could potentially correlate this data to track users across different platforms or apps.
-
Potential Regulatory Concerns: Depending on jurisdiction, location and user profiling data have special handling requirements under laws like GDPR or CCPA. Timezone data might be considered personal data if it contributes to identifying an individual.
-
Unintended Data Exposure: Since HTTP headers are often included in many requests, sending timezone publicly or to third-party services without explicit user consent may expose this data unnecessarily.
Mitigation steps include:
- Only send timezone data when necessary and with user consent.
- Consider anonymizing or obfuscating timezone data if precise location isn't needed.
- Use encrypted connections (HTTPS) to prevent interception.
- Document the use and purpose of timezone data in the privacy policy.
In summary, while sending timezone data is less invasive than sending precise location, it still carries privacy risks related to profiling and location inference that should be carefully managed.
Review TimeZoneInterceptor: Assess and Mitigate Privacy Implications of Timezone Header
The addition of the user’s timezone header can expose approximate location and enable user profiling or cross-service tracking. Please review and implement appropriate safeguards:
• File & Lines
app/src/main/java/com/sopt/clody/data/remote/util/TimeZoneInterceptor.kt(lines 33–34)
• Key privacy concerns
- User profiling: narrows down geographic region, can be combined with other identifiers
- Location inference: timezone boundaries can reveal more precise location
- Cross-service tracking: consistent headers across domains enable fingerprinting
- Regulatory risk: timezone may be “personal data” under GDPR/CCPA if it aids identification
- Unintended exposure: headers sent to third-party services without consent
• Recommended actions
- Only add the timezone header on endpoints that truly require it.
- Obtain explicit user consent before including timezone in requests.
- Consider obfuscating or rounding the timezone if full precision isn’t needed.
- Ensure all requests use HTTPS to prevent header interception.
- Confirm on the server side that timezone data is handled securely (not logged or stored unnecessarily).
- Document the collection and use of timezone data in your privacy policy or user-facing disclosures.
Please address these points to align with privacy best practices and relevant regulations.
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/data/remote/util/TimeZoneInterceptor.kt at
lines 33-34, the current code adds the user's timezone header to all requests,
which raises privacy concerns. Modify the interceptor to add the timezone header
only for specific endpoints that require it, check for explicit user consent
before including the header, and consider obfuscating or rounding the timezone
value to reduce precision. Also, ensure all requests are made over HTTPS to
protect the header in transit. Finally, coordinate with backend to handle
timezone data securely and update privacy documentation accordingly.
| import com.sopt.clody.data.remote.util.NetworkUtil | ||
| import com.sopt.clody.domain.repository.NotificationRepository | ||
| import com.sopt.clody.presentation.utils.extension.TimePeriod | ||
| import com.sopt.clody.presentation.utils.extension.convertUTZtoKST |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the function name accuracy.
The import convertUTZtoKST suggests converting from "UTZ" (possibly User Time Zone) to KST, but this naming could be confusing. Ensure the function name accurately reflects the conversion direction and consider using more descriptive names like convertUserTimeToKST.
🏁 Script executed:
#!/bin/bash
# Description: Verify the actual implementation and naming of the timezone conversion function
# Expected: Find the function definition to understand the conversion direction
ast-grep --pattern 'fun convertUTZtoKST($$$) {
$$$
}'Length of output: 57
🏁 Script executed:
#!/bin/bash
# Search for the definition and usages of convertUTZtoKST to verify its direction and naming
rg -n "convertUTZtoKST" -C 3 .Length of output: 5295
Rename convertUTZtoKST for clarity
The convertUTZtoKST utility indeed reads from the system default (user) timezone and converts that time to KST. However, “UTZ” is non-standard and can be easily confused with “UTC.” Consider renaming this function to more clearly express its behavior, for example:
convertLocalTimeToKSTconvertUserTimeToKST
Locations to update:
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt(line 39)- All call sites in
NotificationSettingViewModel.ktandTimeReminderViewModel.kt
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
around line 39, rename the function convertUTZtoKST to a clearer name such as
convertLocalTimeToKST or convertUserTimeToKST to better reflect that it converts
from the system default timezone to KST. Then update all call sites of this
function in NotificationSettingViewModel.kt and TimeReminderViewModel.kt
accordingly to use the new function name.
| isDraftAlarm = _draftAlarm.value, | ||
| isReplyAlarm = _replyAlarm.value, | ||
| time = time, | ||
| time = convertUTZtoKST(timePeriod, hour, minute), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify timezone conversion function robustness
The conversion from user's timezone to KST using convertUTZtoKST is correct for API communication. However, please verify that the conversion function handles edge cases properly:
- Timezone transitions (DST changes)
- Invalid input parameters
- Timezone offset calculations
🏁 Script executed:
#!/bin/bash
# Find and examine the convertUTZtoKST function implementation
ast-grep --pattern 'fun convertUTZtoKST($_, $_, $_) { $$$ }'
# Also check for any error handling in the conversion functions
fd . -name "TimeZoneExt.kt" | head -1 | xargs catLength of output: 343
🏁 Script executed:
#!/bin/bash
# 1. Locate the implementation of convertUTZtoKST
rg --context 5 'fun convertUTZtoKST' -n
# 2. Find and print the contents of TimeZoneExt.kt (first 200 lines) to inspect any helper functions or error handling
find . -type f -name 'TimeZoneExt.kt' -print -exec sed -n '1,200p' {} \;Length of output: 3182
Fix convertUTZtoKST conversion logic and formatting
The current implementation mistakenly concatenates the hour string with 12 (e.g. "1" + 12 → "112") and uses %02s for formatting, which doesn’t zero-pad numbers. Update the function as follows:
• File: app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
• Method: convertUTZtoKST
fun convertUTZtoKST(timePeriod: TimePeriod, hour: String, minute: String): String {
val userZoneId = ZoneId.systemDefault()
val kstZoneId = ZoneId.of("Asia/Seoul")
- val hour24 = when (timePeriod) {
- TimePeriod.AM -> if (hour == "12") 0 else hour.toInt()
- TimePeriod.PM -> if (hour == "12") 12 else (hour + 12).toInt()
- }
+ val hour24 = when (timePeriod) {
+ TimePeriod.AM -> if (hour == "12") 0 else hour.toInt()
+ TimePeriod.PM -> if (hour == "12") 12 else hour.toInt() + 12
+ }
val userTime = LocalTime.of(hour24, minute.toInt())
val userZoned = ZonedDateTime.of(LocalDate.now(userZoneId), userTime, userZoneId)
val kstZoned = userZoned.withZoneSameInstant(kstZoneId)
val kstHour = kstZoned.hour
val kstMinute = kstZoned.minute
- return String.format("%02s:%02s", kstHour, kstMinute)
+ return String.format("%02d:%02d", kstHour, kstMinute)
}Additional recommendations:
- Change
hourandminuteparameters toIntto avoid unhandledNumberFormatException. - Optionally wrap parsing in a
try/catchor validate inputs before calling.
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt
at line 162, the call to convertUTZtoKST passes hour and minute as strings
causing incorrect concatenation and formatting issues. To fix this, update the
convertUTZtoKST function in
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt to
accept hour and minute as Int parameters, correctly perform arithmetic for time
conversion, and use proper zero-padding formatting (e.g., %02d). Also, add input
validation or try/catch around parsing to handle potential NumberFormatException
safely.
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
Show resolved
Hide resolved
| val isAvailableDay = if (userTimeZone == "Asia/Seoul") { | ||
| selected == today || selected == today.minusDays(1) | ||
| } else { | ||
| selected == today | ||
| } |
There was a problem hiding this comment.
깜빡하고 이 부분도 해당 커밋에 합쳐버렸는데, 전날 일기 작성 불가능하게 분기처리 해놓은 부분입니다!
- referenceDate 주입으로 안정성 증가 (동일 기준일자 보장, 날짜 경계 오류 방, 디버깅 및 테스트 용이) - hour, minute 을 Int -> String 으로 타입 및 포맷 변경
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt (1)
47-50: Add input validation for hour and minute parameters.The previous review comment about hour validation hasn't been addressed. The function assumes valid input but doesn't validate that hour is within the valid range (1-12) or that minute can be parsed as an integer.
Apply this diff to add comprehensive input validation:
val hour24 = when (timePeriod) { - TimePeriod.AM -> if (hour == "12") 0 else hour.toInt() - TimePeriod.PM -> if (hour == "12") 12 else hour.toInt() + 12 + TimePeriod.AM -> if (hour == "12") 0 else { + val h = hour.toIntOrNull() ?: throw IllegalArgumentException("Invalid hour format: $hour") + require(h in 1..11) { "Invalid hour for AM: $hour. Must be 1-12." } + h + } + TimePeriod.PM -> if (hour == "12") 12 else { + val h = hour.toIntOrNull() ?: throw IllegalArgumentException("Invalid hour format: $hour") + require(h in 1..11) { "Invalid hour for PM: $hour. Must be 1-12." } + h + 12 + } }
🧹 Nitpick comments (1)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt (1)
16-17: Consider adding input validation or error handling.The function parses the time string without validation. While
DateTimeFormatter.parsewill throw an exception for invalid formats, consider adding explicit validation or wrapping in a try-catch for better error handling.val formatter = DateTimeFormatter.ofPattern("HH:mm") - val localTime = LocalTime.parse(time, formatter) + val localTime = try { + LocalTime.parse(time, formatter) + } catch (e: Exception) { + throw IllegalArgumentException("Invalid time format. Expected HH:mm format, got: $time", e) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt(1 hunks)app/src/main/res/values-ko/strings.xml(1 hunks)app/src/main/res/values/strings.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/res/values/strings.xml
- app/src/main/res/values-ko/strings.xml
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
[warning] 33-33: String.format("%02d", minute) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
⏰ 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 (1)
app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt (1)
12-60: Good improvements addressing previous feedback.The developer has successfully addressed the major concerns from previous reviews:
- Added
referenceDateparameter to both functions to handle date boundary edge cases- Fixed locale issue in
convertUTZtoKSTby usingLocale.ROOT- The core time zone conversion logic is sound and handles the KST ↔ user timezone conversions correctly
| } | ||
|
|
||
| val hourFormatted = hour12.toString() | ||
| val minuteFormatted = String.format("%02d", minute) |
There was a problem hiding this comment.
Fix implicit default locale usage.
The static analysis correctly identifies that String.format without an explicit locale can cause issues in different regions.
Apply this diff to specify the locale explicitly:
- val minuteFormatted = String.format("%02d", minute)
+ val minuteFormatted = String.format(java.util.Locale.ROOT, "%02d", minute)📝 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 minuteFormatted = String.format("%02d", minute) | |
| val minuteFormatted = String.format(java.util.Locale.ROOT, "%02d", minute) |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 33-33: String.format("%02d", minute) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
at line 33, the use of String.format without specifying a locale can lead to
inconsistent formatting across different regions. Fix this by explicitly passing
Locale.US (or another appropriate locale) as the first argument to String.format
to ensure consistent behavior regardless of the device's locale settings.
| TimePeriod.PM -> if (hour == "12") 12 else hour.toInt() + 12 | ||
| } | ||
|
|
||
| val userTime = LocalTime.of(hour24, minute.toInt()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for minute parameter.
The minute parameter is converted to integer without validation, which could cause a NumberFormatException.
Apply this diff to add minute validation:
- val userTime = LocalTime.of(hour24, minute.toInt())
+ val minuteInt = minute.toIntOrNull() ?: throw IllegalArgumentException("Invalid minute format: $minute")
+ require(minuteInt in 0..59) { "Invalid minute: $minute. Must be 0-59." }
+ val userTime = LocalTime.of(hour24, minuteInt)📝 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 userTime = LocalTime.of(hour24, minute.toInt()) | |
| val minuteInt = minute.toIntOrNull() | |
| ?: throw IllegalArgumentException("Invalid minute format: $minute") | |
| require(minuteInt in 0..59) { "Invalid minute: $minute. Must be 0-59." } | |
| val userTime = LocalTime.of(hour24, minuteInt) |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/utils/extension/TimeZoneExt.kt
at line 52, the minute parameter is converted to an integer without validation,
risking a NumberFormatException. Add input validation to ensure the minute
string can be safely converted to an integer before calling LocalTime.of. If the
conversion fails or the value is out of the valid range (0-59), handle the error
appropriately, such as by returning a default value or throwing a controlled
exception.
2 participants
📌 ISSUE
closed #309
📄 Work Description
✨ PR Point
일일이 할까하다가 API 개수가 상당해서 .. Interceptor가 편할것 같아 진행했는데
일일이 하는게 바람직하다면 말씀해주십시오 ㅠㅠ 수정해보겠습니다.
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor