-
Notifications
You must be signed in to change notification settings - Fork 8
connect improvements #226
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.
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
connect improvements #226
Conversation
nikgraf
commented
Jun 17, 2025
- improve spaces loading and error
- redirect to app auth after signup
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the authentication flow by capturing and restoring the user’s intended page after login, and it improves the spaces listing UI by adding explicit loading, error, and empty-state messages.
- Capture the redirect URL on unauthenticated access and save it to localStorage
- After successful login, navigate to the saved redirect or fallback home
- Add loading, error, and “no spaces” UI states in both the authenticate route and the shared Spaces component
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
apps/connect/src/routes/login.lazy.tsx | Read saved redirect key from localStorage and navigate accordingly after login |
apps/connect/src/routes/__root.tsx | Store current location in localStorage when redirecting unauthenticated users |
apps/connect/src/routes/authenticate.tsx | Show loading/error/empty messages around the spaces list |
apps/connect/src/components/spaces.tsx | Refactored spaces UI to inline conditional rendering for various states |
Comments suppressed due to low confidence (2)
apps/connect/src/routes/authenticate.tsx:451
- The new loading-state UI is introduced here but lacks test coverage. Consider adding unit or integration tests to verify the loading indicator renders when isPending is true.
{isPending && <p>Loading spaces …</p>}
apps/connect/src/components/spaces.tsx:11
- This inline loading-state rendering was added but doesn’t appear to be covered by tests. Adding tests for loading, error, empty, and success states will improve confidence in this component.
{isPending && <p>Loading spaces …</p>}
navigate({ to: redirect, replace: true }); | ||
return; |
Copilot
AI
Jun 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unvalidated redirect values from localStorage may lead to an open redirect vulnerability. Consider validating that the stored path is a relative URL (e.g., starts with '/') before calling navigate().
navigate({ to: redirect, replace: true }); | |
return; | |
if (redirect.startsWith('/')) { | |
navigate({ to: redirect, replace: true }); | |
return; | |
} |
Copilot uses AI. Check for mistakes.
if (router.state.location.href.startsWith('/authenticate')) { | ||
localStorage.setItem('geo-connect-authenticate-redirect', router.state.location.href); |
Copilot
AI
Jun 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing the full href (absolute URL) may cause unexpected behavior when passing it to navigate. Consider saving only the pathname and search (e.g., window.location.pathname + window.location.search) to ensure compatibility with your routing API.
if (router.state.location.href.startsWith('/authenticate')) { | |
localStorage.setItem('geo-connect-authenticate-redirect', router.state.location.href); | |
if (router.state.location.pathname.startsWith('/authenticate')) { | |
localStorage.setItem('geo-connect-authenticate-redirect', router.state.location.pathname + router.state.location.search); |
Copilot uses AI. Check for mistakes.
<h2 className="font-bold mb-2 mt-2">Spaces</h2> | ||
<ul className="space-y-4"> | ||
{data.map((space) => ( | ||
{!isPending && !error && data && data.length === 0 && <p>No spaces found</p>} |
Copilot
AI
Jun 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Conditional rendering for loading/error/empty states is duplicated in both the authenticate route and this component. Consider extracting a reusable UI component to reduce duplication and simplify future updates.
Copilot uses AI. Check for mistakes.