Change the UI for date picker on timeline screen #149
Conversation
WalkthroughThis pull request introduces a new horizontal date picker component for the journey timeline feature. The implementation includes a new Changes
Sequence DiagramsequenceDiagram
participant User
participant HorizontalDatePicker
participant JourneyTimelineViewModel
participant JourneyTimelineScreen
User->>HorizontalDatePicker: Select Date
HorizontalDatePicker->>JourneyTimelineViewModel: setSelectedDate()
JourneyTimelineViewModel->>JourneyTimelineViewModel: Update weekStartDate
JourneyTimelineViewModel->>JourneyTimelineViewModel: setContainsToday()
JourneyTimelineViewModel->>JourneyTimelineScreen: Update Timeline
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (9)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (6)
33-38: Ensure consistency in naming for parameters.
Currently,selectedTimestampis an optional parameter set by default toSystem.currentTimeMillis(). This aligns well with typical usage, but consider consistently naming time-based params (e.g.,selectedTimestampvs.currentTimestamp) to reduce potential confusion and clarify their roles.
44-46: Consider generating the date range once.
var selectedDate by remember { mutableLongStateOf(currentTimestamp) }is set, thengenerateDateRange(selectedDate)is called on each recomposition. If frequent re-compositions happen, generating the range repeatedly might cause slight performance overhead. Caching or memoizing the range might help if the logic becomes more expensive in the future.
59-63: Clarify date click behavior with a preview or hint.
Upon date selection,onDateClickis invoked andselectedDateis updated. It might be helpful to add a subtle transition or highlight to visually confirm the new selection, improving the user’s experience.
74-77: Ensure locale usage is correct for date display.
UsingLocale.getDefault()is typically fine, but consider supporting different locales or allowing the locale to be overridden for edge cases where the user’s device locale differs from the app’s intended display.
90-92: Allow theming or styling customization for selected/unselected states.
The background color and padding differ based on selection state. Exposing these as parameters or theming attributes may improve the reusability of this component in different themes.
116-128: Document or rename the function for clarity on range direction.
generateDateRangereturns a list of 365 days going backward from the selected date (reverse chronological order). Consider clarifying via function name or doc comment that it’s generating past dates, or parameterize the direction.app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineScreen.kt (3)
99-119: Improve discoverability of date-picker toggle.
TogglingshowDatePickeronly via a TextButton is fine, but custom icons or textual hints (“Select Date”) might make it more intuitive for users to find and open the date picker.
137-146: Use more descriptive label than “Today”.
When the user has selected a past day, tapping "Today" resets the date to the current day. Consider labeling it “Reset to Today” or a similarly descriptive phrase to make its action clearer.
157-175: Consider user feedback for offline mode interactions.
When offline, the date picker and timeline content are hidden in favor of the No Internet screen. Consider whether you still want to allow local date filtering in offline mode to view cached data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineScreen.kt(5 hunks)
🔇 Additional comments (1)
app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineScreen.kt (1)
149-153: Validate thatselectedTimeTosuits your generation logic.
HorizontalDatePickeruses up to 365 days from the provided timestamp. Ensure that the difference betweenselectedTimeToand the user’s typical “from” date is well-handled, especially if the timeline data extends beyond one year.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (2)
62-66: Add content padding to LazyRow.The LazyRow might clip items at the edges. Consider adding contentPadding to ensure proper spacing.
LazyRow( horizontalArrangement = Arrangement.spacedBy(8.dp), modifier = Modifier.fillMaxWidth(), - reverseLayout = true + reverseLayout = true, + contentPadding = PaddingValues(horizontal = 16.dp) )
51-55: Remove unnecessary AnimatedVisibility wrapper.The AnimatedVisibility wrapper with
visible = trueis unnecessary as it's always visible.Consider removing the AnimatedVisibility wrapper or implement a proper visibility state if animations are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineScreen.kt(7 hunks)app/src/main/res/values/strings.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineScreen.kt
🔇 Additional comments (1)
app/src/main/res/values/strings.xml (1)
311-311: LGTM!The new string resource follows the proper naming convention and is appropriately placed in the strings.xml file.
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (2)
95-96: 🛠️ Refactor suggestionMove SimpleDateFormat instances outside the composable.
Creating SimpleDateFormat instances on each recomposition is inefficient.
+private val dayOfWeekFormatter = SimpleDateFormat("EEE", Locale.getDefault()) +private val dayFormatter = SimpleDateFormat("dd", Locale.getDefault()) @Composable fun DateCard( date: Long, isSelected: Boolean, onClick: () -> Unit ) { - val dayOfWeek = SimpleDateFormat("EEE", Locale.getDefault()).format(date) - val day = SimpleDateFormat("dd", Locale.getDefault()).format(date) + val dayOfWeek = dayOfWeekFormatter.format(date) + val day = dayFormatter.format(date)
113-125: 🛠️ Refactor suggestionAdd content description for accessibility.
The date card lacks content description for screen readers.
Text( text = dayOfWeek, style = AppTheme.appTypography.label3, - modifier = Modifier.padding(4.dp), + modifier = Modifier + .padding(4.dp) + .semantics { + contentDescription = "Day of week $dayOfWeek" + }, color = if (isSelected) AppTheme.colorScheme.textInversePrimary else AppTheme.colorScheme.textPrimary ) Text( text = day, style = AppTheme.appTypography.label1, fontWeight = FontWeight.Bold, color = if (isSelected) AppTheme.colorScheme.textInversePrimary else AppTheme.colorScheme.textPrimary, - modifier = Modifier.padding(horizontal = 8.dp) + modifier = Modifier + .padding(horizontal = 8.dp) + .semantics { + contentDescription = "Day $day" + } )
🧹 Nitpick comments (3)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (3)
58-62: Remove redundant AnimatedVisibility.The
visibleparameter is hardcoded totrue, making the animation logic redundant. Consider either removing it or adding a parameter to control visibility.- AnimatedVisibility( - visible = true, - enter = slideInVertically { -it } + fadeIn(), - exit = slideOutVertically { -it } + fadeOut() - ) { + Box(modifier = modifier) {
63-68: Simplify layout hierarchy.The Box wrapper with padding is unnecessary as LazyRow could handle the padding directly.
- Box( - modifier = modifier - .fillMaxWidth() - .wrapContentHeight() - .padding(16.dp) - ) { + LazyRow( + state = listState, + horizontalArrangement = Arrangement.spacedBy(8.dp), + modifier = modifier + .fillMaxWidth() + .wrapContentHeight() + .padding(16.dp), + reverseLayout = true + ) {
49-50: Add state restoration for selectedDate.The
selectedDatestate should be restorable to handle configuration changes.- var selectedDate by remember { mutableLongStateOf(currentTimestamp) } + var selectedDate by rememberSaveable { mutableLongStateOf(currentTimestamp) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt(1 hunks)
🔇 Additional comments (2)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (2)
1-40: LGTM! Well-organized imports and constants.The imports are comprehensive and properly organized. The DAYS_IN_YEAR constant is appropriately defined as a private constant.
130-151: 🛠️ Refactor suggestionOptimize date range generation for better performance.
The current implementation has several potential issues:
- Creates a large list of 731 dates (2 years worth) which could impact memory usage
- Reuses the calendar instance which could lead to thread-safety issues
- Could be more efficient using sequence for lazy evaluation
Consider this more efficient implementation:
-fun generateDateRange(selectedDate: Long): List<Long> { +fun generateDateRange(selectedDate: Long): List<Long> = sequence { val calendar = Calendar.getInstance() calendar.timeInMillis = selectedDate - - val dateRange = mutableListOf<Long>() - - repeat(DAYS_IN_YEAR) { + + // Generate past dates + sequence { calendar.add(Calendar.DATE, -1) - dateRange.add(0, calendar.timeInMillis) + yield(calendar.timeInMillis) } - - dateRange.add(selectedDate) - + .take(DAYS_IN_YEAR) + .toList() + .asReversed() + .forEach { yield(it) } + + // Add selected date + yield(selectedDate) + + // Generate future dates calendar.timeInMillis = selectedDate - - repeat(DAYS_IN_YEAR) { + sequence { calendar.add(Calendar.DATE, 1) - dateRange.add(calendar.timeInMillis) - } - - return dateRange -} + yield(calendar.timeInMillis) + } + .take(DAYS_IN_YEAR) + .forEach { yield(it) } +}.toList()Likely invalid or redundant comment.
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (2)
57-63: Consider refining the key in LaunchedEffect.Using
LaunchedEffect(selectedDate, Unit)might be overly broad or unclear. Typically, you want to provide only the dependencies that trigger recomposition. If you want to run the effect whenselectedDatechanges, you can omit theUnitargument or pass it in as an additional key for a specific reason.- LaunchedEffect(selectedDate, Unit) { + LaunchedEffect(selectedDate) { val selectedIndex = currentList.indexOf(selectedDate) ... }
66-78: Define a strategy to bound or purge the date list.This logic continuously appends new dates at either end, which can lead to excessive growth of the in-memory list as the user scrolls further. Consider introducing a maximum bound or a purging mechanism to maintain performance and memory usage.
app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/component/CalenderDataSource.kt (4)
14-18: Consider using a more descriptive method name
The method namegetDatais quite generic and may not convey its purpose clearly. Renaming it to something likegenerateCalendarUiModelorfetchCalendarDatawould provide better clarity.
40-45: Leverage Kotlin idioms for date range generation
Using Java Streams is acceptable, but in Kotlin, consider using a more idiomatic approach with ranges or sequences for improved readability and consistency. For example:-fun getDatesBetween(startDate: LocalDate, endDate: LocalDate): List<LocalDate> { - val numOfDays = ChronoUnit.DAYS.between(startDate, endDate) + 1 - return Stream.iterate(startDate) { date -> date.plusDays(1) } - .limit(numOfDays) - .collect(Collectors.toList()) +fun getDatesBetween(startDate: LocalDate, endDate: LocalDate): List<LocalDate> { + val daysToGenerate = ChronoUnit.DAYS.between(startDate, endDate).toInt() + return (0..daysToGenerate).map { offset -> + startDate.plusDays(offset.toLong()) + } }
66-78: Avoid naming collisions with JavaDate
The nested data classDatecould create confusion with the standard JavaDate. Consider renaming it to something likeCalendarDate, or a more domain-specific name, to reduce ambiguity.
71-77: Consider localization for day formatting
Using a fixed pattern"EE"produces a short day-of-week string in the default locale. To better handle internationalization, specify an explicit locale (e.g.,Locale.getDefault()) or allow the caller to manage it. This ensures the correct language/format for diverse user settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/component/CalenderDataSource.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (1)
128-144: Add content descriptions for accessibility.Providing labels for screen readers ensures this UI is more accessible. You can wrap the clickable element (or each
Text) with asemantics { contentDescription = ... }block to convey the meaning of the day to visually impaired users.Column( horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.Center, modifier = Modifier .background(if (date.isSelected) AppTheme.colorScheme.primary else AppTheme.colorScheme.secondaryInverseVariant) .padding(8.dp) + .semantics { + contentDescription = "Date ${date.date}." + } ) { Text( text = date.day, ...app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/component/CalenderDataSource.kt (1)
30-36: Ensure correctness of date range merging
MergingadditionalDateseither before or aftercurrentListis logical. However, confirm that the date order aligns with UI expectations. If the UI expects ascending chronological order, ensure there's no unintended reversal.
cp-megh-l
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineViewModel.kt (1)
222-228: Enhance week start calculation with timezone handling.The current implementation could be improved by:
- Explicitly handling timezones
- Using system/locale settings for first day of week
private fun getStartOfWeekTimestamp(): Long { - val calendar = Calendar.getInstance() + val calendar = Calendar.getInstance(TimeZone.getDefault(), Locale.getDefault()) calendar.set(Calendar.DAY_OF_WEEK, Calendar.MONDAY) calendar.set(Calendar.HOUR_OF_DAY, 0) calendar.set(Calendar.MINUTE, 0) + calendar.set(Calendar.SECOND, 0) + calendar.set(Calendar.MILLISECOND, 0) return calendar.timeInMillis }app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt (1)
80-90: Optimize list updates in LaunchedEffect.The current implementation unnecessarily filters the list on every page change.
LaunchedEffect(pagerState.currentPage) { when (pagerState.currentPage) { 0 -> { - currentList = (currentList).distinct().filter { it <= LocalDate.now() } + if (currentList.first() > LocalDate.now()) { + currentList = currentList.distinct().filter { it <= LocalDate.now() } + } } currentList.size - 1 -> { - currentList = (currentList).distinct() + if (currentList != currentList.distinct()) { + currentList = currentList.distinct() + } } } }app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineScreen.kt (2)
151-156: Optimize key usage for HorizontalDatePicker.Using
keywithselectedTimeTowill cause the entire DatePicker to recompose on every date selection. Consider using a more stable key or removing it if recomposition is actually needed.- key(state.selectedTimeTo) { + // Only recompose when week changes + key(state.weekStartDate) { HorizontalDatePicker( modifier = Modifier.fillMaxWidth(), selectedTimestamp = state.selectedTimeTo ) }
83-108: Extract common text styling logic.The current implementation duplicates text styling and modifier code. Consider extracting common styles to reduce duplication.
+ @Composable + private fun DateHeader( + text: String, + style: TextStyle, + modifier: Modifier = Modifier + ) { + Text( + text = text, + style = style.copy(fontWeight = FontWeight.Bold), + color = AppTheme.colorScheme.textPrimary, + textAlign = TextAlign.Center, + modifier = modifier.fillMaxWidth() + ) + } Column { if (state.isCurrentUserTimeline) { - Text( - text = state.selectedTimeFrom.formattedMessageDateHeader(LocalContext.current), - style = AppTheme.appTypography.subTitle1.copy(fontWeight = FontWeight.Bold), - color = AppTheme.colorScheme.textPrimary, - textAlign = TextAlign.Center, - modifier = Modifier.fillMaxWidth() + DateHeader( + text = state.selectedTimeFrom.formattedMessageDateHeader(LocalContext.current), + style = AppTheme.appTypography.subTitle1 ) } else { // Similar refactoring for the else block } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/src/main/res/drawable/ic_calendar.pngis excluded by!**/*.png
📒 Files selected for processing (4)
app/src/main/java/com/canopas/yourspace/YourSpaceApplication.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/component/HorizontalDatePicker.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineScreen.kt(6 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/journey/timeline/JourneyTimelineViewModel.kt(5 hunks)
🔇 Additional comments (1)
app/src/main/java/com/canopas/yourspace/YourSpaceApplication.kt (1)
56-56: Verify the intentionality of disabling Crashlytics.The change hardcodes Crashlytics collection to
falsefor all environments. This could impact the ability to monitor and debug production issues. Consider keeping the previous implementation that usedBuildConfig.DEBUGto enable crash reporting in production while disabling it in debug builds.- FirebaseCrashlytics.getInstance().isCrashlyticsCollectionEnabled = false + FirebaseCrashlytics.getInstance().isCrashlyticsCollectionEnabled = !BuildConfig.DEBUG✅ Verification successful
Crashlytics configuration change appears intentional
The change aligns with the project's documented Firebase services, which only include Authentication and Firestore. No crash reporting service is listed in the project setup requirements or configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for Crashlytics-related code echo "=== Searching for Crashlytics usage ===" rg -l "FirebaseCrashlytics" echo -e "\n=== Searching for Firebase config files ===" fd -e json -e xml firebase fd google-services.json echo -e "\n=== Checking for documentation about crash reporting ===" fd -e md -e txt | xargs rg -l -i "crash|firebase|analytics" echo -e "\n=== Searching for privacy-related configuration ===" rg -l "isCrashlyticsCollectionEnabled"Length of output: 775
Script:
#!/bin/bash echo "=== Checking README.md for crash reporting info ===" rg -A 5 -B 5 -i "crash|firebase" README.md echo -e "\n=== Checking firebase.json configuration ===" cat firebase.json echo -e "\n=== Checking YourSpaceApplication.kt context ===" rg -A 5 -B 5 "FirebaseCrashlytics" app/src/main/java/com/canopas/yourspace/YourSpaceApplication.ktLength of output: 2658
| private fun setContainsToday() { | ||
| val today = System.currentTimeMillis() | ||
| val endOfWeek = state.value.weekStartDate | ||
| val containsToday = today in state.value.weekStartDate..endOfWeek | ||
| _state.update { it.copy(containsToday = containsToday) } | ||
| } |
There was a problem hiding this comment.
Fix incorrect date range check in setContainsToday.
The current implementation has a logic error in checking if today falls within the week:
endOfWeekis incorrectly usingweekStartDateinstead of calculating the actual end of week- The range check is incorrect as it uses the same date for both start and end
private fun setContainsToday() {
val today = System.currentTimeMillis()
- val endOfWeek = state.value.weekStartDate
- val containsToday = today in state.value.weekStartDate..endOfWeek
+ val endOfWeek = state.value.weekStartDate + 7 * 24 * 60 * 60 * 1000 // Add 7 days in milliseconds
+ val containsToday = today in state.value.weekStartDate..endOfWeek
_state.update { it.copy(containsToday = containsToday) }
}📝 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 fun setContainsToday() { | |
| val today = System.currentTimeMillis() | |
| val endOfWeek = state.value.weekStartDate | |
| val containsToday = today in state.value.weekStartDate..endOfWeek | |
| _state.update { it.copy(containsToday = containsToday) } | |
| } | |
| private fun setContainsToday() { | |
| val today = System.currentTimeMillis() | |
| val endOfWeek = state.value.weekStartDate + 7 * 24 * 60 * 60 * 1000 // Add 7 days in milliseconds | |
| val containsToday = today in state.value.weekStartDate..endOfWeek | |
| _state.update { it.copy(containsToday = containsToday) } | |
| } |
| val initialSelectedDate = selectedTimestamp.let { | ||
| LocalDate.ofEpochDay(it / (24 * 60 * 60 * 1000)) | ||
| } |
There was a problem hiding this comment.
Add null safety for timestamp conversion.
The timestamp to LocalDate conversion could fail with invalid timestamps.
- val initialSelectedDate = selectedTimestamp.let {
- LocalDate.ofEpochDay(it / (24 * 60 * 60 * 1000))
- }
+ val initialSelectedDate = try {
+ selectedTimestamp.let {
+ LocalDate.ofEpochDay(it / (24 * 60 * 60 * 1000))
+ }
+ } catch (e: Exception) {
+ LocalDate.now()
+ }📝 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 initialSelectedDate = selectedTimestamp.let { | |
| LocalDate.ofEpochDay(it / (24 * 60 * 60 * 1000)) | |
| } | |
| val initialSelectedDate = try { | |
| selectedTimestamp.let { | |
| LocalDate.ofEpochDay(it / (24 * 60 * 60 * 1000)) | |
| } | |
| } catch (e: Exception) { | |
| LocalDate.now() | |
| } |
Changelog
New Features
Screen_recording_20250103_105314.mp4
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI Changes
Chores