Conversation
✅ Deploy Preview for redi-love ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Great work on this refactor and bug fix, Abrar! Centralizing the routing logic into the root layout and adding the auth-redirect flow makes the app’s feel much more robust and professional.
The only major issue is I did run into a bug and it got stuck on the loading page, make sure to address that issue and some other minor issues. But awesome job!
There was a problem hiding this comment.
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
| await signOutUser(); | ||
| console.log('Sign out successful'); | ||
| setShowSignOutSheet(false); | ||
| router.replace('/home' as any); |
There was a problem hiding this comment.
Nice fix here to avoid route the user twice
|
|
||
| export default function AuthLayout() { | ||
| useEffect(() => { | ||
| SplashScreen.hideAsync(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think keeping it as is should be ok
| variant="secondary" | ||
| title="Cannot change email address" | ||
| onPress={() => {}} | ||
| onPress={() => { }} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's true; this is a remnant from when we were discussing not tying it NetIDs
There was a problem hiding this comment.
yeah let me know what clement say about this, we can add that to the next PR thing if needs to be fixed
There was a problem hiding this comment.
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)
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
| } | ||
|
|
||
| await refreshProfile(); | ||
|
|
There was a problem hiding this comment.
Awesome catch adding await refreshProfile() here before routing! This is a great way to prevent any weird split-second loading glitches or stale data.
| /> | ||
| </Stack> | ||
| {showOnboarding && ( | ||
| <OnboardingVideo |
There was a problem hiding this comment.
I noticed we moved the OnboardingVideo logic from index.tsx over to home.tsx
Just a quick thought: if a brand-new user joins via a deep link, they usually go straight from create-profile to the main app tabs, and might skip the home page entirely. Will they miss the onboarding video in that case? I'm not sure if we want all new users to see it regardless of how they join, but just wanted to check if this move was intentional
There was a problem hiding this comment.
The onboarding video is more of a first time install thing; a user should not be able to get a sign-in link until they have launched the app.
frontend/app/auth-redirect.tsx
Outdated
| // _layout.tsx handles the actual sign-in and routes away when done. | ||
| export default function AuthRedirect() { | ||
| useEffect(() => { | ||
| }, []); |
There was a problem hiding this comment.
we have an empty useEffect hook here. Did you have plans to put some deep-link logic in here originally? If not, we can probably just delete those lines to keep the code clean since empty useEffect hook does nothing.
There was a problem hiding this comment.
I was toying around with the splash screen stuff and removed the code in here lmfao but not the effect
| }; | ||
|
|
||
| useEffect(() => { | ||
| SplashScreen.hideAsync(); |
There was a problem hiding this comment.
Just like the other layout file, since SplashScreen.hideAsync() returns a promise, we might want to add a quick .catch() here just to keep the promise handling consistent across the app
| const handleVideoFinish = () => { | ||
| const handleVideoFinish = async () => { | ||
| await markOnboardingVideoAsShown(); | ||
| setShowVideo(false); |
There was a problem hiding this comment.
Just a small thought on the handleVideoFinish timing. Right now we are await the save before we call setShowVideo(false). If the storage write is a bit slow, the user might see the video stay on the screen for a second after it ends.
We could probably move setShowVideo(false) to the top of the function so the video disappears instantly while the save happens in the background! But also i don't think it's a big issue, so totally up to you to change the order or not.
| import LoadingSpinner from './components/ui/LoadingSpinner'; | ||
|
|
||
| export default function Index() { | ||
| const [showVideo, setShowVideo] = useState(false); |
There was a problem hiding this comment.
again, is it intentional we moved the show video logic out of here to home.tsx, and will it change any behaviors of showing the video and everything
There was a problem hiding this comment.
For some reason or another, the deep link takes a user to the index page, so I changed the index page to function as a loading screen.
Simulator.Screen.Recording.-.iPhone.17.-.2026-03-08.at.21.07.02.movHere's the new behavior for using expired links |
|
laylaliuuu
left a comment
There was a problem hiding this comment.
Thank you for addressing all the comments so quickly!! I was testing the bug you fixed, it seems good!! the only one minor thing is let’s say if the user is logged in, and they clicked on the invalid link to open the link they are automitaclly logged out, which is kinda annoying (just in case if they ever click on the wrong link and accidentally clicked “open”) do you think we should change that
|
yyay |




Summary
Simulator.Screen.Recording.-.iPhone.17.-.2026-03-07.at.21.24.59.mov
Simulator.Screen.Recording.-.iPhone.17.-.2026-03-07.at.21.23.51.mov
Simulator.Screen.Recording.-.iPhone.17.-.2026-03-07.at.21.20.07.mov
Simulator.Screen.Recording.-.iPhone.17.-.2026-03-07.at.21.19.37.mov