feat(auth): implement ProtectedRoute with signup completion checks#1236
feat(auth): implement ProtectedRoute with signup completion checks#1236tyler-dane merged 2 commits intomainfrom
Conversation
- Added ProtectedRoute component to manage user authentication and redirect logic based on signup completion status. - Integrated useAuthCheck and useHasCompletedSignup hooks to determine user state and redirect to appropriate routes. - Created unit tests for ProtectedRoute to validate redirect behavior for authenticated and unauthenticated users. - Updated routing constants to include ONBOARDING path for better navigation handling. - Enhanced onboarding flow to ensure users are guided correctly based on their signup status.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a ProtectedRoute component with signup completion checks and enhances the onboarding flow to support different behaviors based on whether users are on the /login or /onboarding route.
- Adds authentication-aware redirect logic to ProtectedRoute based on signup completion status
- Implements route-based onboarding flow that skips certain steps for authenticated users re-doing onboarding
- Adds a "Re-do onboarding" command to the command palette
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/web/src/views/Onboarding/constants/onboarding.constants.ts |
Defines onboarding step IDs and steps to skip for authenticated users |
packages/web/src/views/Onboarding/OnboardingFlow.tsx |
Implements route-based filtering and authentication-aware step filtering |
packages/web/src/views/Onboarding/OnboardingFlow.test.tsx |
Adds comprehensive tests for route-based behavior |
packages/web/src/views/CmdPalette/CmdPalette.tsx |
Adds "Re-do onboarding" command |
packages/web/src/common/constants/routes.ts |
Adds ONBOARDING route constant |
packages/web/src/auth/ProtectedRoute.tsx |
Implements signup completion-based redirect logic |
packages/web/src/auth/ProtectedRoute.test.tsx |
Adds tests for ProtectedRoute redirect scenarios |
…te handling - Removed the renderWithRouter utility in ProtectedRoute tests, directly using the render function for clarity. - Added a new test case to ensure that the ProtectedRoute does not redirect when hasCompletedSignup is null, addressing the loading state scenario. - Updated ProtectedRoute component to wait for hasCompletedSignup to be determined before performing any redirects, improving user experience during authentication checks.
| expect(screen.getByTestId("initial-step-index")).toHaveTextContent("0"); | ||
| expect(screen.getByTestId("first-step-id")).toHaveTextContent( | ||
| "welcome-screen", |
There was a problem hiding this comment.
According to the project's testing guidelines in AGENTS.md, tests should not use data-testid attributes or CSS selectors to locate elements. Instead, use semantic locators and roles (e.g., getByRole, getByLabelText, getByText) that reflect how users interact with the application. Consider refactoring the mocked Onboarding component to expose test content through semantic queries.
| expect(screen.getByTestId("onboarding")).toBeInTheDocument(); | ||
| // Should start with welcome-screen (onboarding step), not welcome (login step) | ||
| expect(screen.getByTestId("first-step-id")).toHaveTextContent( | ||
| "welcome-screen", | ||
| ); | ||
| expect(screen.getByTestId("first-step-id")).not.toHaveTextContent( | ||
| /^welcome$/, |
There was a problem hiding this comment.
According to the project's testing guidelines in AGENTS.md, tests should not use data-testid attributes. Use semantic locators like getByRole or getByText instead to query elements the way users would interact with them.
| expect(screen.getByTestId("initial-step-index")).toHaveTextContent("0"); | ||
| expect(screen.getByTestId("first-step-id")).toHaveTextContent( | ||
| "welcome-screen", |
There was a problem hiding this comment.
According to the project's testing guidelines in AGENTS.md, tests should not use data-testid attributes. Use semantic locators instead to reflect how users interact with the application.
|
|
||
| // Should have fewer steps (filtered out Google login and migration steps) | ||
| const totalSteps = parseInt( | ||
| screen.getByTestId("total-steps").textContent || "0", |
There was a problem hiding this comment.
According to the project's testing guidelines in AGENTS.md, tests should not use data-testid attributes. Use semantic locators like getByRole or getByText instead.
|
|
||
| // Should skip to sign-in-with-google step | ||
| const initialStepIndex = parseInt( | ||
| screen.getByTestId("initial-step-index").textContent || "0", |
There was a problem hiding this comment.
According to the project's testing guidelines in AGENTS.md, tests should not use data-testid attributes. Use semantic locators to query elements instead.
| expect(screen.getByTestId("first-step-id")).toHaveTextContent( | ||
| "sign-in-with-google", |
There was a problem hiding this comment.
According to the project's testing guidelines in AGENTS.md, tests should not use data-testid attributes. Use semantic locators instead.
| expect(screen.getByTestId("onboarding")).toBeInTheDocument(); | ||
| expect(screen.getByTestId("first-step-id")).toHaveTextContent( | ||
| "welcome", |
There was a problem hiding this comment.
According to the project's testing guidelines in AGENTS.md, tests should not use data-testid attributes. Use semantic locators to reflect how users interact with the application.
Closes #1235