Skip to content

Fix some routing issues #112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laylaliuuu merged 9 commits into from
Mar 9, 2026
Merged

Fix some routing issues #112

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1415857
Fix deleting profile issue
abrar-amin Mar 7, 2026
a850e24
Update create-profile.tsx
abrar-amin Mar 8, 2026
a17c12d
Added loading screen for login and rearranged page routing
abrar-amin Mar 8, 2026
5a1bee5
Update account-settings.tsx
abrar-amin Mar 8, 2026
c2895fa
Merge origin/main into fix-some-routing-issues, resolve conflicts
abrar-amin Mar 8, 2026
90463f1
Restore loading spinner to index.tsx lost during merge resolution
abrar-amin Mar 8, 2026
ee4b197
Update _layout.tsx
abrar-amin Mar 8, 2026
8887534
Update account-settings.tsx
abrar-amin Mar 8, 2026
a578c21
Address comments in PR
abrar-amin Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion frontend/app/(auth)/(tabs)/profile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
const [animationTrigger, setAnimationTrigger] = useState(0);

useThemeAware(); // Force re-render when theme changes
const { profile, loading } = useProfile();

Check warning on line 54 in frontend/app/(auth)/(tabs)/profile.tsx

View workflow job for this annotation

GitHub Actions / Frontend - Build & Type Check 

'loading' is assigned a value but never used
Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Just a tiny clean-up note here, it looks like the CI is flagging "loading" from useProfile(). It would be great to remove it just to keep the file tidy and make sure no CI check warning

Copy link
Collaborator Author

@abrar-amin abrar-amin Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

const [showSignOutSheet, setShowSignOutSheet] = useState(false);
const [showRatingSheet, setShowRatingSheet] = useState(false);
const [showOnboardingVideo, setShowOnboardingVideo] = useState(false);
Expand All @@ -73,7 +73,6 @@
await signOutUser();
console.log('Sign out successful');
setShowSignOutSheet(false);
router.replace('/home' as any);
Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix here to avoid route the user twice

Copy link
Collaborator Author

@abrar-amin abrar-amin Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yipee

} catch (error) {
console.error('Sign out error:', error);
Alert.alert(
Expand Down
6 changes: 6 additions & 0 deletions frontend/app/(auth)/_layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import * as SplashScreen from 'expo-splash-screen';
import { Stack } from 'expo-router';
import { useEffect } from 'react';
import { NotificationsProvider } from '../contexts/NotificationsContext';

export default function AuthLayout() {
useEffect(() => {
SplashScreen.hideAsync();
Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a super minor thing, since SplashScreen.hideAsync() returns a promise, it might be safer to just add a quick .catch() to the end of it, just to prevent any unhandled promise rejection, but I don't think it will actually cause any issue, totally up to you to change it or not

Copy link
Collaborator Author

@abrar-amin abrar-amin Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think keeping it as is should be ok

Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sounds good

}, []);

return (
<NotificationsProvider>
<Stack
Expand Down
10 changes: 3 additions & 7 deletions frontend/app/(auth)/account-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import DeleteAccountSheet from '@/app/components/ui/DeleteAccountSheet';
import ListItem from '@/app/components/ui/ListItem';
import ListItemWrapper from '@/app/components/ui/ListItemWrapper';
import SignOutSheet from '@/app/components/ui/SignOutSheet';
import { router } from 'expo-router';
import { LogOut, Pencil, Trash2 } from 'lucide-react-native';
import React, { useEffect, useState } from 'react';
import { Alert, ScrollView, StatusBar, StyleSheet, View } from 'react-native';
Expand Down Expand Up @@ -62,13 +61,12 @@ export default function AccountSettingsPage() {
await signOutUser();
console.log('Sign out successful');
setShowSignOutSheet(false);
router.replace('/home' as any);
} catch (error) {
console.error('Sign out error:', error);
Alert.alert(
'Error',
'Failed to sign out: ' +
(error instanceof Error ? error.message : 'Unknown error')
(error instanceof Error ? error.message : 'Unknown error')
);
}
};
Expand Down Expand Up @@ -111,15 +109,13 @@ export default function AccountSettingsPage() {
await signOutUser();
setShowDeleteSheet(false);
setIsDeleting(false);

router.replace('/home' as any);
} catch (error) {
console.error('Delete account error:', error);
setIsDeleting(false);
Alert.alert(
'Error',
'Failed to delete account: ' +
(error instanceof Error ? error.message : 'Unknown error')
(error instanceof Error ? error.message : 'Unknown error')
);
}
};
Expand Down Expand Up @@ -157,7 +153,7 @@ export default function AccountSettingsPage() {
iconLeft={Pencil}
variant="secondary"
title="Cannot change email address"
onPress={() => {}}
onPress={() => { }}
Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at this email address section where we have the empty onPress={() => {}}. Since we can't actually edit the email right now, it might be a bit confusing for users if it looks and feels like a clickable button but doesn't do anything when tapped.

Perhaps instead of a clickable row, we could just display their email as static text, maybe with a little gray subtitle underneath saying 'Email cannot be changed'? I think that might make the UI feel a bit more intuitive. If it's not too hard to swap out, I think that would be a good UX polish.

Copy link
Collaborator Author

@abrar-amin abrar-amin Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true; this is a remnant from when we were discussing not tying it NetIDs

Copy link
Collaborator Author

@abrar-amin abrar-amin Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will defer to @clementroze on this

Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah let me know what clement say about this, we can add that to the next PR thing if needs to be fixed

Copy link
Collaborator

@clementroze clementroze Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think it looks or feels like a clickable button because its disabeld (muted + gray) and there are no press states (bc of the disabled state)

see here -
image

i think we should still keep it bc its typical to be able to change your email in apps. Since we dont allow it because we have a special sign in with Cornell SSO case, we should still show which email they're signed in on for visibility.

i also think we should follow the pattern of rather still showing something, but making it disabled, rather than just hiding or removing it completely

lmk what yall think

/>
</ListItemWrapper>
</View>
Expand Down
6 changes: 5 additions & 1 deletion frontend/app/(auth)/create-profile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,13 @@
import AppInput from '../components/ui/AppInput';
import AppText from '../components/ui/AppText';
import Button from '../components/ui/Button';
import CityAutocomplete from '../components/ui/CityAutocomplete';
import ListItem from '../components/ui/ListItem';
import ListItemWrapper from '../components/ui/ListItemWrapper';
import CityAutocomplete from '../components/ui/CityAutocomplete';
import SearchableDropdown from '../components/ui/SearchableDropdown';
import Sheet from '../components/ui/Sheet';
import Tag from '../components/ui/Tag';
import { useProfile } from '../contexts/ProfileContext';
import { useThemeAware } from '../contexts/ThemeContext';
import { useHapticFeedback } from '../hooks/useHapticFeedback';
import { useOnboardingState } from '../hooks/useOnboardingState';
Expand Down Expand Up @@ -157,6 +158,7 @@
export default function CreateProfileScreen() {
useThemeAware(); // Force re-render when theme changes
const haptic = useHapticFeedback();
const { refreshProfile } = useProfile();

const [currentStep, setCurrentStep] = useState(2); // Start at step 2
const [direction, setDirection] = useState<'forward' | 'backward'>('forward');
Expand Down Expand Up @@ -204,7 +206,7 @@
useNativeDriver: true,
}),
]).start();
}, [currentStep]);

Check warning on line 209 in frontend/app/(auth)/create-profile.tsx

View workflow job for this annotation

GitHub Actions / Frontend - Build & Type Check 

React Hook useEffect has missing dependencies: 'direction', 'fadeAnim', and 'slideAnim'. Either include them or remove the dependency array
Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it looks like the CI is flagging a missing dependency warning here. We should probably add them to the dependency to make sure React doesn't accidentally use stale states and cause the UI to jitter.

Copy link
Collaborator Author

@abrar-amin abrar-amin Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is from a previous PR, but I can fix it


if (!isLoaded) {
return null; // Wait for AsyncStorage to load
Expand Down Expand Up @@ -376,6 +378,8 @@
console.error('Failed to save preferences:', prefError);
}

await refreshProfile();

Copy link
Collaborator

@laylaliuuu laylaliuuu Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome catch adding await refreshProfile() here before routing! This is a great way to prevent any weird split-second loading glitches or stale data.

// Clear storage and navigate to main app
await clearStorage();
router.replace('/(auth)/(tabs)' as any);
Expand Down
Loading
Loading