Implemented UI for workout history page#108
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Workout History feature: new navigation route and navigation handling, a Compose screen with calendar and list modes, ViewModel state for grouped history and per-day maps, UI updates for tab icons and history rows, visibility changes on history composables, and four new vector drawable resources. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant VM as ProfileViewModel
participant Nav as RootNavigationRepository
participant Screen as WorkoutHistoryScreen
participant Tab as UpliftTabRow
participant Views as Calendar/List Views
User->>VM: call toHistory()
VM->>Nav: navigate(UpliftRootRoute.WorkoutHistory)
Nav->>Screen: show WorkoutHistoryScreen
Screen->>VM: collect ProfileUiState (historyListItems, workoutDates)
Screen->>Tab: render tabs (Calendar, List)
User->>Tab: select mode
Tab->>Views: request render (Calendar or List)
Views->>User: display history items / dropdown
User->>Screen: press back
Screen->>Nav: onBack -> popBackStack()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt (2)
135-141: Sort history before grouping to keep month sections and rows deterministic.
groupBypreserves encounter order, so unsorted input can produce unstable month ordering and item ordering in the list.Proposed refactor
- val groupedItems = remember(historyItems) { - historyItems.groupBy { + val groupedItems = remember(historyItems) { + historyItems + .sortedByDescending { it.timestamp } + .groupBy { val date = Instant.ofEpochMilli(it.timestamp) .atZone(ZoneId.systemDefault()) .toLocalDate() date.format(DateTimeFormatter.ofPattern("MMMM yyyy", Locale.US)) } }Also applies to: 151-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt` around lines 135 - 141, The month grouping logic in WorkoutHistoryScreen uses historyItems.groupBy which preserves input order, causing non-deterministic month and row ordering; sort historyItems by timestamp before grouping (e.g., replace uses of historyItems.groupBy in the groupedItems calculation and the other grouping at the later block with historyItems.sortedBy { it.timestamp }.groupBy { /* same date logic */ }) so both groupedItems and the other grouping produce stable, deterministic month sections and item order.
85-86: Localize UI strings and date formatting instead of hardcoding English/US values.This screen currently hardcodes user-facing labels and forces U.S. month formatting, which will render incorrectly for non-English locales.
Also applies to: 122-122, 140-140, 207-207, 235-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt` around lines 85 - 86, Replace hardcoded UI labels and US-only date formatting with localized resources and locale-aware formatters: swap the inline strings in the tabs list (currently "Calendar", "List") for string resources (e.g., R.string.calendar, R.string.list) and use those resource IDs wherever user-facing text is hardcoded (also at the other affected spots). Replace any DateTimeFormatter or SimpleDateFormat instances forced to Locale.US with a locale-aware formatter (e.g., DateTimeFormatter.ofLocalizedDate(...) or android.text.format.DateFormat.getDateFormat(context) / DateFormat.getMediumDateInstance(Locale.getDefault())) so month and date order follow the device locale; ensure the code paths that build labels (the same places referenced by tabs/icons and the other mentioned lines) read context resources and Locale.getDefault() rather than hardcoded English/US values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/components/profile/workouts/HistorySection.kt`:
- Around line 74-78: The divider condition is comparing against the full
historyItems list instead of the visible subset; fix by materializing the
displayed list (e.g., val visibleItems = historyItems.take(5)) and iterate over
visibleItems.forEachIndexed { index, historyItem -> ... } then use index !=
visibleItems.size - 1 (or index != visibleItems.lastIndex) when deciding to
render HorizontalDivider in HistorySection (affecting the
HistoryItemRow/HorizontalDivider rendering logic).
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`:
- Around line 116-118: The tap targets for the navigation icons in
WorkoutHistoryScreen are too small (currently using Modifier.size(24.dp) or 16dp
for previous/next month controls); update each icon container (the modifiers
that call .clickable { ... } — e.g., the back action using onBack() and the
previous/next month actions) to use a 48.dp touch target (e.g.,
Modifier.size(48.dp).clickable { ... }) while keeping the visual Icon content at
24.dp so the visual size stays the same but accessibility/tap area is increased.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`:
- Around line 135-141: The month grouping logic in WorkoutHistoryScreen uses
historyItems.groupBy which preserves input order, causing non-deterministic
month and row ordering; sort historyItems by timestamp before grouping (e.g.,
replace uses of historyItems.groupBy in the groupedItems calculation and the
other grouping at the later block with historyItems.sortedBy { it.timestamp
}.groupBy { /* same date logic */ }) so both groupedItems and the other grouping
produce stable, deterministic month sections and item order.
- Around line 85-86: Replace hardcoded UI labels and US-only date formatting
with localized resources and locale-aware formatters: swap the inline strings in
the tabs list (currently "Calendar", "List") for string resources (e.g.,
R.string.calendar, R.string.list) and use those resource IDs wherever
user-facing text is hardcoded (also at the other affected spots). Replace any
DateTimeFormatter or SimpleDateFormat instances forced to Locale.US with a
locale-aware formatter (e.g., DateTimeFormatter.ofLocalizedDate(...) or
android.text.format.DateFormat.getDateFormat(context) /
DateFormat.getMediumDateInstance(Locale.getDefault())) so month and date order
follow the device locale; ensure the code paths that build labels (the same
places referenced by tabs/icons and the other mentioned lines) read context
resources and Locale.getDefault() rather than hardcoded English/US values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3da717f9-8788-4c45-ab99-7d970f2a0692
📒 Files selected for processing (9)
app/src/main/java/com/cornellappdev/uplift/ui/MainNavigationWrapper.ktapp/src/main/java/com/cornellappdev/uplift/ui/components/general/UpliftTabRow.ktapp/src/main/java/com/cornellappdev/uplift/ui/components/profile/workouts/HistorySection.ktapp/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/profile/ProfileViewModel.ktapp/src/main/res/drawable/ic_advance_month.xmlapp/src/main/res/drawable/ic_back_month.xmlapp/src/main/res/drawable/ic_calendar_tab.xmlapp/src/main/res/drawable/ic_list_tab.xml
| historyItems.take(5).forEachIndexed { index, historyItem -> | ||
| HistoryItemRow(historyItem = historyItem) | ||
| if (index != historyItems.size - 1) { | ||
| HorizontalDivider(color = GRAY01) | ||
| HorizontalDivider(color = GRAY01, thickness = 1.dp) | ||
| } |
There was a problem hiding this comment.
Divider condition uses the wrong collection length.
Line 76 compares against historyItems.size - 1, but rendering is over historyItems.take(5). When there are more than 5 items, this adds a trailing divider after the last visible row.
Proposed fix
fun HistoryList(
historyItems: List<HistoryItem>,
modifier: Modifier = Modifier
) {
Column(modifier = modifier) {
- historyItems.take(5).forEachIndexed { index, historyItem ->
+ val visibleItems = historyItems.take(5)
+ visibleItems.forEachIndexed { index, historyItem ->
HistoryItemRow(historyItem = historyItem)
- if (index != historyItems.size - 1) {
+ if (index != visibleItems.lastIndex) {
HorizontalDivider(color = GRAY01, thickness = 1.dp)
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/components/profile/workouts/HistorySection.kt`
around lines 74 - 78, The divider condition is comparing against the full
historyItems list instead of the visible subset; fix by materializing the
displayed list (e.g., val visibleItems = historyItems.take(5)) and iterate over
visibleItems.forEachIndexed { index, historyItem -> ... } then use index !=
visibleItems.size - 1 (or index != visibleItems.lastIndex) when deciding to
render HorizontalDivider in HistorySection (affecting the
HistoryItemRow/HorizontalDivider rendering logic).
| viewModel: ProfileViewModel = hiltViewModel(), | ||
| onBack: () -> Unit | ||
| ) { | ||
| val uiState by viewModel.uiStateFlow.collectAsState() |
There was a problem hiding this comment.
Nit: UpliftViewModel has a function called collectUiStateValue. Also, we can update it to use collectAsStateWithLifecycle as well
| uiState: ProfileUiState, | ||
| onBack: () -> Unit | ||
| ) { | ||
| var selectedTab by remember { mutableStateOf(0) } |
There was a problem hiding this comment.
Nit: can use mutableIntStateOf
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.SpaceBetween | ||
| ) { | ||
| Icon( |
There was a problem hiding this comment.
Nit: can use IconButton
| } | ||
|
|
||
| @Composable | ||
| private fun WorkoutHistoryHeader(onBack: () -> Unit) { |
There was a problem hiding this comment.
Nit: there might be an existing top bar in UpliftTopBar file that might serve this purpose. We can also probably edit it to be more generalizable if needed since it might be used across several screens
There was a problem hiding this comment.
Just a follow up on this, but it might be worth making a reusable composable for this header since multiple screens may share this header but just change their title text
| WorkoutHistoryScreenContent(uiState = uiState, onBack = onBack) | ||
| } | ||
|
|
||
| @Composable |
There was a problem hiding this comment.
Should probably have some sort of system padding especially for edge to edge devices
| } | ||
|
|
||
| @Composable | ||
| private fun WorkoutHistoryCalendarView(historyItems: List<HistoryItem>) { |
There was a problem hiding this comment.
It seems like there should be a feature to select the days on the calendar and show a little popup for the ones that have a workout checked in. Is this going to be implemented in a future PR?
There was a problem hiding this comment.
Not sure if I see this added in, but I'll assume it's still being worked on
| // Month Selector | ||
| Row( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.Center, |
There was a problem hiding this comment.
Nit: you could probably do something like Arrangement.spacedBy(24.dp, Alignment.CenterHorizontally) to achieve a similar effect w/o the extra spacers.
| @Composable | ||
| private fun WorkoutHistoryListView(historyItems: List<HistoryItem>) { | ||
| val groupedItems = remember(historyItems) { | ||
| historyItems.groupBy { |
There was a problem hiding this comment.
Might need to add .sortedByDescending { it.timestamp } before grouping since order is not guaranteed.
| } | ||
| } | ||
|
|
||
| LazyColumn( |
There was a problem hiding this comment.
For Lazy Composables, good to add keys.
| ) { | ||
| item { Spacer(modifier = Modifier.height(8.dp)) } | ||
|
|
||
| groupedItems.forEach { (month, items) -> |
There was a problem hiding this comment.
For LazyColumns, it might be better to flatten into a list model instead of nesting for better item recycling:
eg. something like this
sealed class HistoryListItem {
data class Header(val month: String) : HistoryListItem()
data class Item(val data: HistoryItem) : HistoryListItem()
}
val listItems = remember(historyItems) {
buildList {
groupedItems.forEach { (month, items) ->
add(Header(month))
items.forEach { add(Item(it)) }
}
}
}
| private fun WorkoutHistoryListView(historyItems: List<HistoryItem>) { | ||
| val groupedItems = remember(historyItems) { | ||
| historyItems.groupBy { | ||
| val date = Instant.ofEpochMilli(it.timestamp) |
There was a problem hiding this comment.
Probably better to push these sorts of transformations to the vm to be precomputed
| var currentMonth by remember { mutableStateOf(YearMonth.now()) } | ||
| val workoutDates = remember(historyItems) { | ||
| historyItems.map { | ||
| Instant.ofEpochMilli(it.timestamp) |
There was a problem hiding this comment.
Same thing here as other Instant comment
| EmptyHistorySection() | ||
| } | ||
| } else { | ||
| when (selectedTab) { |
There was a problem hiding this comment.
Nit/Enhancement: We can consider using something like AnimatedContent to provide better transitions between the tab contents
AndrewCheung360
left a comment
AndrewCheung360
left a comment
There was a problem hiding this comment.
Overall great work! Left some comments, most of which are minor and some are questions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt (1)
258-280:⚠️ Potential issue | 🟠 MajorIncrease the month navigation tap targets.
The previous/next month controls still attach
clickabledirectly to 16dp icons, making them hard to tap. Wrap each icon in a 48dpIconButtonor centered clickable container while keeping the visual icon at 16dp.♿ Proposed fix
- Icon( - painter = painterResource(id = R.drawable.ic_back_month), - contentDescription = "Previous Month", - modifier = Modifier - .size(16.dp) - .clickable { currentMonth = currentMonth.minusMonths(1) }, - tint = PRIMARY_BLACK - ) + IconButton(onClick = { currentMonth = currentMonth.minusMonths(1) }) { + Icon( + painter = painterResource(id = R.drawable.ic_back_month), + contentDescription = "Previous Month", + modifier = Modifier.size(16.dp), + tint = PRIMARY_BLACK + ) + } ... - Icon( - painter = painterResource(id = R.drawable.ic_advance_month), - contentDescription = "Next Month", - modifier = Modifier - .size(16.dp) - .clickable { currentMonth = currentMonth.plusMonths(1) }, - tint = PRIMARY_BLACK - ) + IconButton(onClick = { currentMonth = currentMonth.plusMonths(1) }) { + Icon( + painter = painterResource(id = R.drawable.ic_advance_month), + contentDescription = "Next Month", + modifier = Modifier.size(16.dp), + tint = PRIMARY_BLACK + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt` around lines 258 - 280, The previous/next month icons in WorkoutHistoryScreen are using .clickable on a 16.dp Icon, making tap targets too small; update the two Icon usages (the painterResource icons with contentDescription "Previous Month" and "Next Month") to wrap the icon in an IconButton or a container with a 48.dp hit target (e.g., apply Modifier.size(48.dp).clickable { currentMonth = currentMonth.minusMonths(1) } / .plusMonths(1) and center the 16.dp Icon inside) so the visual icon remains 16.dp but the touch area meets accessibility guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`:
- Around line 121-141: The trailing Spacer used to visually center the "History"
Text is hardcoded to 24.dp and doesn't match the left IconButton's action slot;
update the Spacer (after Text) to use the same width as the IconButton/action
slot (e.g., Modifier.width(48.dp) or reuse the IconButton's modifier size) so
the title is truly centered—look for IconButton, Text(text = "History") and the
trailing Spacer in WorkoutHistoryScreen.kt and make the Spacer match the action
slot width.
---
Duplicate comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`:
- Around line 258-280: The previous/next month icons in WorkoutHistoryScreen are
using .clickable on a 16.dp Icon, making tap targets too small; update the two
Icon usages (the painterResource icons with contentDescription "Previous Month"
and "Next Month") to wrap the icon in an IconButton or a container with a 48.dp
hit target (e.g., apply Modifier.size(48.dp).clickable { currentMonth =
currentMonth.minusMonths(1) } / .plusMonths(1) and center the 16.dp Icon inside)
so the visual icon remains 16.dp but the touch area meets accessibility
guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de43d7f6-d580-4bee-afa3-9e49190e550d
📒 Files selected for processing (1)
app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt
| ), | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Icon( |
There was a problem hiding this comment.
This and the other icon could probably be made into IconButtons
| private fun WorkoutHistoryListView(historyItems: List<HistoryItem>) { | ||
| val groupedItems = remember(historyItems) { | ||
| historyItems | ||
| .sortedByDescending { it.timestamp } |
There was a problem hiding this comment.
I think you mentioned that you were adding the instant stuff to the vm, but I think the whole grouped items computation with sorting and grouping could probably be moved there to so the ui just receives the ready state
| .toLocalDate() | ||
| }.toSet() | ||
| } | ||
|
|
There was a problem hiding this comment.
This doesn't have to be added for this PR, but I think it would be a nice touch if we add small little animation transitions when switching between months in the calendar view similar to what we have with the tabs with AnimatedContent
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt (2)
383-383: Stray indentation.
@Composableis indented to column ~9 while the surrounding top-level declarations sit at column 0. Harmless, but please reformat soWorkoutCalendarDropdownlines up with the other top-level functions in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt` at line 383, The `@Composable` annotation before WorkoutCalendarDropdown is indented incorrectly; move the "@Composable" line (and ensure the following "fun WorkoutCalendarDropdown" declaration) back to column 0 so the annotation and function are top-level and aligned with other top-level declarations in the file; update indentation for the WorkoutCalendarDropdown declaration to match surrounding functions.
551-600: Preview duplicates the VM's grouping/mapping logic.The preview re-implements the same "sort desc → groupBy month → buildList of Header/Workout/SpacerItem" pipeline plus the
workoutDatesgrouping thatProfileViewModel.loadProfile()already does. Extracting a pure helper (e.g.buildHistoryListItems(items)/buildWorkoutDates(items)in the VM file or a util) and calling it from both places would prevent the two implementations from drifting (different sort/divider rules, locale, zone, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt` around lines 551 - 600, Extract the duplicate grouping/sorting logic into reusable pure helpers (e.g. buildHistoryListItems(items: List<HistoryItem>): List<HistoryListItem> and buildWorkoutDates(items: List<HistoryItem>): Map<LocalDate, List<HistoryItem>>) placed next to ProfileViewModel (or in a util file), implement them to perform the "sort desc → groupBy month → produce Header/Workout/SpacerItem" and the LocalDate grouping using the same ZoneId/Locale/DateTimeFormatter as loadProfile(), then update WorkoutHistoryScreenPreview to call those helpers instead of re-implementing the pipeline and pass their results into ProfileUiState (referencing HistoryItem, HistoryListItem, ProfileUiState, WorkoutHistoryScreenContent and ProfileViewModel.loadProfile to keep behavior identical).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`:
- Line 335: The local val isSelected = selectedDate == date is computed but
never used; either remove it or wire it into the cell so selection visuals are
driven by it. To fix, pass isSelected into CalendarDayCell (e.g., add an
isSelected: Boolean parameter to CalendarDayCell and supply isSelected when
calling it) and update CalendarDayCell's rendering to use that flag for
selected-state visuals; alternatively, if you prefer per-week selection, delete
the unused isSelected declaration. Use the identifiers isSelected, selectedDate,
date, and CalendarDayCell to locate the relevant code.
- Around line 384-440: WorkoutCalendarDropdown's arrowXOffset uses cellWidth =
availableWidth / 7 which assumes equal spacing, but the calendar uses
Arrangement.SpaceBetween with fixed 40.dp cells; update the arrow center
calculation in WorkoutCalendarDropdown to match that layout: compute the cell
center as 20.dp + dayOfWeekIndex * ((availableWidth - 40.dp) / 6) (all in dp,
then convert with density.toPx()) instead of using (cellWidth * dayOfWeekIndex)
+ (cellWidth / 2); change references to arrowXOffset, cellWidth, availableWidth,
dayOfWeekIndex and ensure density conversion happens after the dp-based formula
so the arrow tip aligns with WorkoutHistoryCalendarView's spaced 40.dp cells.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`:
- Line 383: The `@Composable` annotation before WorkoutCalendarDropdown is
indented incorrectly; move the "@Composable" line (and ensure the following "fun
WorkoutCalendarDropdown" declaration) back to column 0 so the annotation and
function are top-level and aligned with other top-level declarations in the
file; update indentation for the WorkoutCalendarDropdown declaration to match
surrounding functions.
- Around line 551-600: Extract the duplicate grouping/sorting logic into
reusable pure helpers (e.g. buildHistoryListItems(items: List<HistoryItem>):
List<HistoryListItem> and buildWorkoutDates(items: List<HistoryItem>):
Map<LocalDate, List<HistoryItem>>) placed next to ProfileViewModel (or in a util
file), implement them to perform the "sort desc → groupBy month → produce
Header/Workout/SpacerItem" and the LocalDate grouping using the same
ZoneId/Locale/DateTimeFormatter as loadProfile(), then update
WorkoutHistoryScreenPreview to call those helpers instead of re-implementing the
pipeline and pass their results into ProfileUiState (referencing HistoryItem,
HistoryListItem, ProfileUiState, WorkoutHistoryScreenContent and
ProfileViewModel.loadProfile to keep behavior identical).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4de46f8-ec11-4100-9128-4836ebce8b6b
📒 Files selected for processing (2)
app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/profile/ProfileViewModel.kt
| private fun WorkoutCalendarDropdown( | ||
| historyItem: HistoryItem, | ||
| dayOfWeekIndex: Int | ||
| ) { | ||
| val screenWidth = LocalConfiguration.current.screenWidthDp.dp | ||
| val horizontalPadding = 16.dp | ||
| val availableWidth = screenWidth - (horizontalPadding * 2) | ||
| val cellWidth = availableWidth / 7 | ||
|
|
||
| // Calculate arrow offset to point to the center of the day cell | ||
| val arrowXOffset = (cellWidth * dayOfWeekIndex) + (cellWidth / 2) | ||
| val density = LocalDensity.current | ||
|
|
||
| val shape = remember(arrowXOffset, density) { | ||
| GenericShape { size, _ -> | ||
| val arrowWidth = with(density) { 12.dp.toPx() } | ||
| val arrowHeight = with(density) { 8.dp.toPx() } | ||
| val cornerRadius = with(density) { 12.dp.toPx() } | ||
| val arrowX = with(density) { arrowXOffset.toPx() } | ||
|
|
||
| // Top edge with arrow | ||
| moveTo(0f, arrowHeight + cornerRadius) | ||
| arcTo( | ||
| rect = Rect(0f, arrowHeight, cornerRadius * 2, arrowHeight + cornerRadius * 2), | ||
| startAngleDegrees = 180f, | ||
| sweepAngleDegrees = 90f, | ||
| forceMoveTo = false | ||
| ) | ||
| lineTo(arrowX - arrowWidth / 2, arrowHeight) | ||
| lineTo(arrowX, 0f) | ||
| lineTo(arrowX + arrowWidth / 2, arrowHeight) | ||
| lineTo(size.width - cornerRadius, arrowHeight) | ||
| arcTo( | ||
| rect = Rect(size.width - cornerRadius * 2, arrowHeight, size.width, arrowHeight + cornerRadius * 2), | ||
| startAngleDegrees = 270f, | ||
| sweepAngleDegrees = 90f, | ||
| forceMoveTo = false | ||
| ) | ||
| // Right edge | ||
| lineTo(size.width, size.height - cornerRadius) | ||
| arcTo( | ||
| rect = Rect(size.width - cornerRadius * 2, size.height - cornerRadius * 2, size.width, size.height), | ||
| startAngleDegrees = 0f, | ||
| sweepAngleDegrees = 90f, | ||
| forceMoveTo = false | ||
| ) | ||
| // Bottom edge | ||
| lineTo(cornerRadius, size.height) | ||
| arcTo( | ||
| rect = Rect(0f, size.height - cornerRadius * 2, cornerRadius * 2, size.height), | ||
| startAngleDegrees = 90f, | ||
| sweepAngleDegrees = 90f, | ||
| forceMoveTo = false | ||
| ) | ||
| close() | ||
| } | ||
| } |
There was a problem hiding this comment.
Arrow pointer can miss the center of the day cell.
cellWidth = availableWidth / 7 assumes the 7 day cells are evenly distributed across the row, but WorkoutHistoryCalendarView lays them out with Arrangement.SpaceBetween and fixed 40.dp cells (lines 270, 329, 347, 515). With SpaceBetween, cell i's center is 20.dp + i * (availableWidth - 40.dp) / 6, not (availableWidth / 7) * (i + 0.5). The two formulas only coincide at i = 3; at the edges the arrow tip can be off by several dp on typical phone widths, so the dropdown pointer visibly misses Monday/Sunday cells.
🔧 Proposed fix
- val screenWidth = LocalConfiguration.current.screenWidthDp.dp
- val horizontalPadding = 16.dp
- val availableWidth = screenWidth - (horizontalPadding * 2)
- val cellWidth = availableWidth / 7
-
- // Calculate arrow offset to point to the center of the day cell
- val arrowXOffset = (cellWidth * dayOfWeekIndex) + (cellWidth / 2)
+ val screenWidth = LocalConfiguration.current.screenWidthDp.dp
+ val horizontalPadding = 16.dp
+ val availableWidth = screenWidth - (horizontalPadding * 2)
+ val cellSize = 40.dp
+
+ // Row uses SpaceBetween with 7 fixed-width cells, so cell i's
+ // center = cellSize/2 + i * (availableWidth - cellSize) / 6
+ val arrowXOffset = (cellSize / 2) +
+ (availableWidth - cellSize) * dayOfWeekIndex / 6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`
around lines 384 - 440, WorkoutCalendarDropdown's arrowXOffset uses cellWidth =
availableWidth / 7 which assumes equal spacing, but the calendar uses
Arrangement.SpaceBetween with fixed 40.dp cells; update the arrow center
calculation in WorkoutCalendarDropdown to match that layout: compute the cell
center as 20.dp + dayOfWeekIndex * ((availableWidth - 40.dp) / 6) (all in dp,
then convert with density.toPx()) instead of using (cellWidth * dayOfWeekIndex)
+ (cellWidth / 2); change references to arrowXOffset, cellWidth, availableWidth,
dayOfWeekIndex and ensure density conversion happens after the dp-based formula
so the arrow tip aligns with WorkoutHistoryCalendarView's spaced 40.dp cells.
| } | ||
|
|
||
|
|
||
| @Composable |
There was a problem hiding this comment.
The formatting looks a little weird here
| // Dropdown | ||
| val selectedInThisWeek = week.find { it == selectedDate } | ||
|
|
||
| AnimatedVisibility( |
There was a problem hiding this comment.
Nice job with the animation! I think one improvement would be to make it so when clicking on another date or the same one when the dropdown is expanded, it shrinks smoothly since right now it does look a little snappy. I think there was an attempt with the shrinkVertically, but it doesn't look like it's occurring on my end. But this is just a minor nitpick, overall nice job!
| .background(GRAY04, CircleShape) | ||
| ) | ||
| Spacer(modifier = Modifier.width(4.dp)) | ||
| val shortDate = remember(historyItem.timestamp) { |
There was a problem hiding this comment.
This Instant should probably be moved to vm or it should already be formatted when passed in to the ui
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt (2)
298-321: Simplify theweeksbuilder.The
currentDay += 7 - (if (list.size == 1) firstDayOfWeek else 0)trick and the paralleldayIndex/currentDaybookkeeping make this harder to reason about than it needs to be. You can build the grid from the total number of cells (firstDayOfWeek + daysInMonth, rounded up to a multiple of 7) and chunk by 7:Proposed refactor
- val daysInMonth = animatedMonth.lengthOfMonth() - val firstOfMonth = animatedMonth.atDay(1) - val firstDayOfWeek = (firstOfMonth.dayOfWeek.value + 6) % 7 - - val weeks = remember(animatedMonth) { - val list = mutableListOf<List<LocalDate?>>() - var currentDay = 1 - - while (currentDay <= daysInMonth) { - val week = (0..6).map { i -> - val dayIndex = list.size * 7 + i - val dayOfMonth = dayIndex - firstDayOfWeek + 1 - - if (dayOfMonth in 1..daysInMonth) { - animatedMonth.atDay(dayOfMonth) - } else null - } - - list.add(week) - currentDay += 7 - (if (list.size == 1) firstDayOfWeek else 0) - } - - list - } + val weeks = remember(animatedMonth) { + val daysInMonth = animatedMonth.lengthOfMonth() + val firstDayOfWeek = (animatedMonth.atDay(1).dayOfWeek.value + 6) % 7 + val totalCells = ((firstDayOfWeek + daysInMonth + 6) / 7) * 7 + (0 until totalCells) + .map { idx -> + val dayOfMonth = idx - firstDayOfWeek + 1 + if (dayOfMonth in 1..daysInMonth) animatedMonth.atDay(dayOfMonth) else null + } + .chunked(7) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt` around lines 298 - 321, The weeks builder is overly complex; replace the current while/currentDay logic inside the remember(animatedMonth) block with a simpler grid approach: compute firstDayOfWeek and daysInMonth, set totalCells = ((firstDayOfWeek + daysInMonth + 6) / 7) * 7, create a sequence 0 until totalCells and map each index to a LocalDate? by computing dayOfMonth = index - firstDayOfWeek + 1 and returning animatedMonth.atDay(dayOfMonth) when in 1..daysInMonth else null, then chunk the resulting list into sublists of 7 to produce weeks. Keep the remember(animatedMonth) and references to animatedMonth, firstDayOfWeek, daysInMonth, and weeks.
575-596: Preview duplicates the grouping logic inProfileViewModel.
buildList { … }in the preview is effectively a copy ofloadProfile'sgrouped/historyListItemspipeline (lines 111-133 inProfileViewModel.kt). Consider extracting the sort/group/flatten to a small top-level helper (e.g.,fun buildHistoryListItems(items: List<HistoryItem>): List<HistoryListItem>) and calling it from both the VM and the preview so the preview can't drift from production behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt` around lines 575 - 596, The preview duplicates the sort/group/flatten logic currently implemented in ProfileViewModel (the grouped/historyListItems pipeline), so extract that pipeline into a single top-level helper function named something like buildHistoryListItems(items: List<HistoryItem>): List<HistoryListItem> and have both the Preview code (where buildList { … } and historyItems are used) and ProfileViewModel.loadProfile call this helper; ensure the helper produces Header, Workout, and SpacerItem entries in the same order as the current buildList usage so the Preview cannot drift from production behaviour.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/ProfileScreen.kt`:
- Around line 172-178: Preview shortDate values in the HistoryItem list don't
match the runtime formatter; either change ProfileViewModel.shortDateFormatter
or update the preview entries in ProfileScreen's historyItems to match the same
pattern. Locate ProfileViewModel.shortDateFormatter and the HistoryItem(...)
initializers in ProfileScreen.kt and make the formats consistent (e.g., if the
formatter is "MMM, d" update preview strings from "Mar 29" to "Mar, 29", or
change the formatter to "MMM d" to match the existing preview).
In
`@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/profile/ProfileViewModel.kt`:
- Line 20: Remove the unintended static import of
java.time.temporal.TemporalQueries.localDate from ProfileViewModel.kt; locate
the import line "import java.time.temporal.TemporalQueries.localDate" near the
top of the file (in the ProfileViewModel class file) and delete it so the file
no longer references an unused TemporalQuery static member.
- Around line 202-205: shortDateFormatter currently uses the pattern "MMM, d"
which inserts an extra comma (e.g., "Mar, 29") causing a visual mismatch with
the previewed shortDate values; change the DateTimeFormatter pattern in
shortDateFormatter (where DateTimeFormatter.ofPattern(...) is used) to "MMM d"
and keep the existing withLocale(Locale.US).withZone(ZoneId.systemDefault())
configuration so runtime-formatted shortDate values match the preview strings.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.kt`:
- Around line 298-321: The weeks builder is overly complex; replace the current
while/currentDay logic inside the remember(animatedMonth) block with a simpler
grid approach: compute firstDayOfWeek and daysInMonth, set totalCells =
((firstDayOfWeek + daysInMonth + 6) / 7) * 7, create a sequence 0 until
totalCells and map each index to a LocalDate? by computing dayOfMonth = index -
firstDayOfWeek + 1 and returning animatedMonth.atDay(dayOfMonth) when in
1..daysInMonth else null, then chunk the resulting list into sublists of 7 to
produce weeks. Keep the remember(animatedMonth) and references to animatedMonth,
firstDayOfWeek, daysInMonth, and weeks.
- Around line 575-596: The preview duplicates the sort/group/flatten logic
currently implemented in ProfileViewModel (the grouped/historyListItems
pipeline), so extract that pipeline into a single top-level helper function
named something like buildHistoryListItems(items: List<HistoryItem>):
List<HistoryListItem> and have both the Preview code (where buildList { … } and
historyItems are used) and ProfileViewModel.loadProfile call this helper; ensure
the helper produces Header, Workout, and SpacerItem entries in the same order as
the current buildList usage so the Preview cannot drift from production
behaviour.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11f44de8-5051-4185-ba7a-4eb12c727ab5
📒 Files selected for processing (4)
app/src/main/java/com/cornellappdev/uplift/ui/components/profile/workouts/HistorySection.ktapp/src/main/java/com/cornellappdev/uplift/ui/screens/profile/ProfileScreen.ktapp/src/main/java/com/cornellappdev/uplift/ui/screens/profile/WorkoutHistoryScreen.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/profile/ProfileViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/cornellappdev/uplift/ui/components/profile/workouts/HistorySection.kt
| val historyItems = listOf( | ||
| HistoryItem("Morrison", "11:00 PM", "March 29, 2024", now - (1 * 24 * 60 * 60 * 1000), "1 day ago"), | ||
| HistoryItem("Noyes", "1:00 PM", "March 29, 2024", now - (3 * 24 * 60 * 60 * 1000), "2 days ago"), | ||
| HistoryItem("Teagle Up", "2:00 PM", "March 29, 2024", now - (7 * 24 * 60 * 60 * 1000), "1 day ago"), | ||
| HistoryItem("Teagle Down", "12:00 PM", "March 29, 2024", now - (15 * 24 * 60 * 60 * 1000), "1 day ago"), | ||
| HistoryItem("Helen Newman", "10:00 AM", "March 29, 2024", now, "Today"), | ||
| HistoryItem("Morrison", "11:00 PM", "March 29, 2024", now, "Today", "Mar 29"), | ||
| HistoryItem("Noyes", "1:00 PM", "March 28, 2024", now - 86400000L, "Yesterday", "Mar 28"), | ||
| HistoryItem("Teagle Up", "2:00 PM", "February 15, 2024", now - 4000000000L, "1 month ago", "Feb 15"), | ||
| HistoryItem("Helen Newman", "9:30 AM", "February 10, 2024", now - 4430000000L, "1 month ago", "Feb 10"), | ||
| HistoryItem("Morrison", "6:45 PM", "February 3, 2024", now - 5030000000L, "1 month ago", "Feb 3") | ||
| ) |
There was a problem hiding this comment.
Preview shortDate strings diverge from the runtime formatter.
The preview uses "Mar 29", "Feb 15", etc. (pattern MMM d), but ProfileViewModel.shortDateFormatter is defined with pattern "MMM, d", so the actual UI will render "Mar, 29". Root cause and fix are discussed on ProfileViewModel.kt lines 202-205.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/ProfileScreen.kt`
around lines 172 - 178, Preview shortDate values in the HistoryItem list don't
match the runtime formatter; either change ProfileViewModel.shortDateFormatter
or update the preview entries in ProfileScreen's historyItems to match the same
pattern. Locate ProfileViewModel.shortDateFormatter and the HistoryItem(...)
initializers in ProfileScreen.kt and make the formats consistent (e.g., if the
formatter is "MMM, d" update preview strings from "Mar 29" to "Mar, 29", or
change the formatter to "MMM d" to match the existing preview).
| import java.time.LocalDate | ||
| import java.time.ZoneId | ||
| import java.time.format.DateTimeFormatter | ||
| import java.time.temporal.TemporalQueries.localDate |
There was a problem hiding this comment.
Likely unintended import.
java.time.temporal.TemporalQueries.localDate is a static TemporalQuery member (not the class LocalDate) and isn't referenced anywhere in this file — probably an accidental IDE auto-import. Please remove it.
Proposed fix
-import java.time.temporal.TemporalQueries.localDate📝 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.
| import java.time.temporal.TemporalQueries.localDate |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/profile/ProfileViewModel.kt`
at line 20, Remove the unintended static import of
java.time.temporal.TemporalQueries.localDate from ProfileViewModel.kt; locate
the import line "import java.time.temporal.TemporalQueries.localDate" near the
top of the file (in the ProfileViewModel class file) and delete it so the file
no longer references an unused TemporalQuery static member.
2 participants
Overview
Implemented the Workout History Page UI according to figma designs.
Changes Made
Implemented Calendar and List tab views of workout history.
Added nav from profile page to workout history page.
Next Steps
Build the workout history viewmodel and implement the drop down info functionality on days where a workout is logged according to designs.
Finish nav and networking.
Test once real workouts are logged.
Screenshots
Workout History UI Preview
WorkoutHistoryUiPreview.mp4
Summary by CodeRabbit
New Features
UI Improvements
Assets