Delete Button#109
Merged
isiahpwilliams merged 8 commits intomainfrom Apr 21, 2026
Merged
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 32 minutes and 13 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)
📝 WalkthroughWalkthroughSession management now persists a Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsScreen
participant DeleteDialog
participant SettingsViewModel
participant UserInfoRepository
participant GraphQL as GraphQL API
participant Firebase
participant SessionManager
participant DataStore
User->>SettingsScreen: Tap "Delete account"
SettingsScreen->>DeleteDialog: show dialog
User->>DeleteDialog: Confirm deletion
DeleteDialog->>SettingsViewModel: onDeleteAccount()
SettingsViewModel->>UserInfoRepository: deleteAccount()
UserInfoRepository->>SessionManager: read userId
UserInfoRepository->>GraphQL: DeleteUserMutation(userId)
GraphQL-->>UserInfoRepository: response (success/error)
UserInfoRepository->>Firebase: attempt Firebase user deletion
Firebase-->>UserInfoRepository: deletion result (logged)
UserInfoRepository->>SessionManager: logout()
UserInfoRepository->>DataStore: clear user data
UserInfoRepository-->>SettingsViewModel: return success/failure
SettingsViewModel->>SettingsScreen: navigate to Onboarding (on success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
🤖 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/data/repositories/UserInfoRepository.kt`:
- Around line 243-255: In deleteAccount(), the caught exception from
firebaseAuth.currentUser?.delete()?.await() currently allows execution to
continue and return true; change the flow so a failure to delete from the
provider aborts account teardown: when the delete await throws, log the error
(keep Log.e) and return false (or rethrow) immediately instead of proceeding to
sessionManager.logout(), dataStore.edit { it.clear() }, and the final Log.d;
ensure the success path only reaches those calls and returns true when
firebaseAuth.currentUser?.delete()?.await() completes without exception.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/profile/SettingsViewModel.kt`:
- Around line 58-66: The delete-account failure path in
SettingsViewModel.onDeleteAccount is currently silent; update the ViewModel to
surface errors by adding an error field or one-shot event to SettingsUiState
(e.g., settingsError: String? or a SingleLiveEvent/Channel) and set it when
userInfoRepository.deleteAccount() returns false, then consume it in the UI to
show a Snackbar/dialog; also log the failure and clear or reset the one-shot
event after it is displayed. Ensure references: onDeleteAccount,
userInfoRepository.deleteAccount, rootNavigationRepository.navigate, and
SettingsUiState are updated so the UI receives and displays the error.
🪄 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: c26a61a0-953d-4b62-9e74-06a238984f80
📒 Files selected for processing (7)
app/src/main/java/com/cornellappdev/uplift/data/auth/SessionManager.ktapp/src/main/java/com/cornellappdev/uplift/data/auth/TokenManager.ktapp/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.ktapp/src/main/java/com/cornellappdev/uplift/ui/components/profile/DeleteDialog.ktapp/src/main/java/com/cornellappdev/uplift/ui/screens/profile/SettingsScreen.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/profile/SettingsViewModel.ktapp/src/main/res/drawable/ic_trash_2.xml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt (1)
61-77:⚠️ Potential issue | 🟠 MajorInitial
StateFlowemission causes duplicate back-stack entry on startup.
sessionManager.isLoggedInis aStateFlowwith eager sharing, so it emits its current value immediately upon collection. This triggers the initialnavEventemission in theviewModelScope.launchblock (line 73), which fires before any user interaction occurs.Since
MainNavigationWrappercallsnavController.navigate(it)unconditionally at line 121 withoutlaunchSingleToporpopUpTo, the navigation to the same destination asNavHost'sstartDestinationadds a duplicate back-stack entry (e.g., Home → Home). Users can then press back from the home screen unintentionally.Consider one of:
- Skip the first emission (e.g.,
.drop(1)) to only emitnavEventon real login/logout/delete transitions.- Track the previous
isLoggedInvalue and only emit when it actually transitions.- Add
popUpTo(..) { inclusive = true }at the navigation site to make re-navigation idempotent.🔧 Example: only emit on transitions
viewModelScope.launch { - sessionManager.isLoggedIn.collect { loggedIn -> + var previous: Boolean? = null + sessionManager.isLoggedIn.collect { loggedIn -> applyMutation { copy(isLoggedIn = loggedIn) } val hasSkipped = userInfoRepository.getSkipFromDataStore() val shouldShowHome = loggedIn || hasSkipped || !ONBOARDING_FLAG val newRoute = if (shouldShowHome) UpliftRootRoute.Home else UpliftRootRoute.Onboarding - applyMutation { - copy( - startDestination = newRoute, - navEvent = UIEvent(newRoute) - ) - } + val isTransition = previous != null && previous != loggedIn + previous = loggedIn + applyMutation { + copy( + startDestination = newRoute, + navEvent = if (isTransition) UIEvent(newRoute) else navEvent + ) + } } }🤖 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/nav/RootNavigationViewModel.kt` around lines 61 - 77, The view model currently reacts to the eager initial emission from sessionManager.isLoggedIn and emits a navEvent causing duplicate navigation; change the collection to skip the initial emission (e.g., use sessionManager.isLoggedIn.drop(1).collect { ... }) inside the viewModelScope.launch so applyMutation that sets startDestination and navEvent only runs on real transitions, preventing the duplicate back-stack entry (alternatively, you can track previous isLoggedIn and only emit when it changes or handle idempotent navigation at MainNavigationWrapper by using popUpTo/launchSingleTop).
🧹 Nitpick comments (3)
app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/SettingsScreen.kt (2)
45-45: ConsiderrememberSaveableso the dialog survives configuration changes.Using
remember { mutableStateOf(false) }means the dialog will be dismissed on rotation or process death restoration. If that's undesirable, switch torememberSaveable.♻️ Proposed change
- var showDeleteDialog by remember { mutableStateOf(false)} + var showDeleteDialog by rememberSaveable { mutableStateOf(false) }🤖 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/SettingsScreen.kt` at line 45, The dialog visibility state uses remember { mutableStateOf(false) } which won't survive configuration changes; change the declaration of showDeleteDialog in SettingsScreen (the var showDeleteDialog by remember { mutableStateOf(false) }) to use rememberSaveable (e.g., rememberSaveable { mutableStateOf(false) }) and add the necessary import for androidx.compose.runtime.saveable.rememberSaveable so the dialog state persists across rotations and process restoration.
111-161: DRY:LogOutButtonandDeleteAccountButtonare near-duplicates.The two composables differ only in icon, content description, label, and icon tint. Consider extracting a shared
SettingsActionButton(icon, label, iconTint, onClick)to reduce duplication and keep styling consistent as the design system evolves.♻️ Sketch
`@Composable` private fun SettingsActionButton( `@DrawableRes` icon: Int, label: String, iconTint: Color = Color.Unspecified, textColor: Color = AppColors.Red, onClick: () -> Unit, ) { Row( modifier = Modifier.clickable(onClick = onClick), horizontalArrangement = Arrangement.spacedBy(8.dp), verticalAlignment = Alignment.CenterVertically, ) { Icon(painterResource(id = icon), contentDescription = label, tint = iconTint) Text( text = label, fontSize = 16.sp, fontFamily = montserratFamily, fontWeight = FontWeight.Medium, color = textColor, ) } }🤖 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/SettingsScreen.kt` around lines 111 - 161, LogOutButton and DeleteAccountButton are near-duplicates; extract a single reusable composable (e.g., SettingsActionButton) that accepts parameters for icon resource id, content description/label, iconTint, textColor, and onClick, then replace both LogOutButton and DeleteAccountButton to call SettingsActionButton with their specific icon, label, tint, and click handler; keep the common Row layout, spacing, font (montserratFamily), fontSize (16.sp), and fontWeight (FontWeight.Medium) in the new SettingsActionButton so styling remains centralized.app/src/main/java/com/cornellappdev/uplift/ui/components/goalsetting/DeleteDialog.kt (1)
73-81: Replace hardcodedoffsetwithAlign/padding and clean up the modifier chain.A few issues in this call site:
- The position
.offset(x = 206.dp, y = 12.dp)is hardcoded against the Card'swidth(250.dp)and iconsize(32.dp). Any future change to the dialog width or close-icon size silently breaks the layout. The sibling caller inui/components/profile/DeleteDialog.ktalready uses the idiomaticModifier.align(Alignment.TopEnd).padding(top = 12.dp, end = 12.dp)pattern — worth mirroring here for consistency.- The chain
.size(32.dp).\n clickable(...)with the trailing dot on its own line is valid Kotlin but easy to misread as a typo; standard style is to start each call with the leading dot.offsetapplied afterclickablemeans the click region is translated visually but the node is still laid out at (0,0) inside the Box, which also explains why an explicit offset is needed instead of proper alignment.♻️ Proposed refactor
- CloseButton(modifier = Modifier - .size(32.dp). - clickable( - interactionSource = remember { MutableInteractionSource() }, - indication = null - ) { - onDismiss() - } - .offset(x = 206.dp, y = 12.dp)) + CloseButton( + modifier = Modifier + .align(Alignment.TopEnd) + .padding(top = 12.dp, end = 12.dp) + .size(32.dp) + .clickable( + interactionSource = remember { MutableInteractionSource() }, + indication = null + ) { + onDismiss() + } + )🤖 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/goalsetting/DeleteDialog.kt` around lines 73 - 81, The CloseButton placement uses a hardcoded offset and a confusing modifier chain; replace the .offset(...) positioning with idiomatic alignment and padding (use Modifier.align(Alignment.TopEnd).padding(top = 12.dp, end = 12.dp)) and reorder the modifier calls so each chained call starts with a leading dot and clickable follows size (e.g., .size(32.dp).clickable(...)) to ensure the visual position and click target match; update the CloseButton call in DeleteDialog.kt to use Modifier.align(Alignment.TopEnd).padding(...) plus the existing remember { MutableInteractionSource() } and indication = null, and remove the hardcoded .offset and stray trailing dot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt`:
- Around line 61-77: The view model currently reacts to the eager initial
emission from sessionManager.isLoggedIn and emits a navEvent causing duplicate
navigation; change the collection to skip the initial emission (e.g., use
sessionManager.isLoggedIn.drop(1).collect { ... }) inside the
viewModelScope.launch so applyMutation that sets startDestination and navEvent
only runs on real transitions, preventing the duplicate back-stack entry
(alternatively, you can track previous isLoggedIn and only emit when it changes
or handle idempotent navigation at MainNavigationWrapper by using
popUpTo/launchSingleTop).
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/uplift/ui/components/goalsetting/DeleteDialog.kt`:
- Around line 73-81: The CloseButton placement uses a hardcoded offset and a
confusing modifier chain; replace the .offset(...) positioning with idiomatic
alignment and padding (use Modifier.align(Alignment.TopEnd).padding(top = 12.dp,
end = 12.dp)) and reorder the modifier calls so each chained call starts with a
leading dot and clickable follows size (e.g., .size(32.dp).clickable(...)) to
ensure the visual position and click target match; update the CloseButton call
in DeleteDialog.kt to use Modifier.align(Alignment.TopEnd).padding(...) plus the
existing remember { MutableInteractionSource() } and indication = null, and
remove the hardcoded .offset and stray trailing dot.
In
`@app/src/main/java/com/cornellappdev/uplift/ui/screens/profile/SettingsScreen.kt`:
- Line 45: The dialog visibility state uses remember { mutableStateOf(false) }
which won't survive configuration changes; change the declaration of
showDeleteDialog in SettingsScreen (the var showDeleteDialog by remember {
mutableStateOf(false) }) to use rememberSaveable (e.g., rememberSaveable {
mutableStateOf(false) }) and add the necessary import for
androidx.compose.runtime.saveable.rememberSaveable so the dialog state persists
across rotations and process restoration.
- Around line 111-161: LogOutButton and DeleteAccountButton are near-duplicates;
extract a single reusable composable (e.g., SettingsActionButton) that accepts
parameters for icon resource id, content description/label, iconTint, textColor,
and onClick, then replace both LogOutButton and DeleteAccountButton to call
SettingsActionButton with their specific icon, label, tint, and click handler;
keep the common Row layout, spacing, font (montserratFamily), fontSize (16.sp),
and fontWeight (FontWeight.Medium) in the new SettingsActionButton so styling
remains centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98f43585-323b-4346-9a3a-ce585174bb15
📒 Files selected for processing (6)
app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.ktapp/src/main/java/com/cornellappdev/uplift/ui/components/goalsetting/DeleteDialog.ktapp/src/main/java/com/cornellappdev/uplift/ui/components/profile/DeleteDialog.ktapp/src/main/java/com/cornellappdev/uplift/ui/screens/profile/SettingsScreen.ktapp/src/main/java/com/cornellappdev/uplift/ui/theme/Color.ktapp/src/main/java/com/cornellappdev/uplift/ui/viewmodels/nav/RootNavigationViewModel.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/cornellappdev/uplift/ui/theme/Color.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/cornellappdev/uplift/ui/components/profile/DeleteDialog.kt
- app/src/main/java/com/cornellappdev/uplift/data/repositories/UserInfoRepository.kt
AndrewCheung360
approved these changes
Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Created a delete account button in the settings page
Changes Made
Implemented the design for the button and the popup from Figma (SettingsScreen.kt and DeleteDialog.kt)
Implemented networking for delete and fixed networking for log out (UserInfoRepository.kt and SettingsViewModel.kt)
Test Coverage
Manually tested the log out and delete functionality
Summary by CodeRabbit
New Features
Changes