Skip to content

Conversation

@rebelchris
Copy link
Contributor

@rebelchris rebelchris commented Mar 26, 2025

Changes

Vibe coding made some changes to keep email in onboarding context

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Preview domain

https://vibe-auth-context.preview.app.daily.dev

@vercel
Copy link

vercel bot commented Mar 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp 🛑 Canceled (Inspect) Apr 3, 2025 6:41am
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Apr 3, 2025 6:41am

Copy link
Contributor

Copilot AI left a 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 pull request refactors the handling of email state in the authentication flows by introducing an AuthDataContext and updating related components to use the new context provider. Key changes include:

  • Creation of AuthDataContext and AuthDataProvider to manage email state.
  • Removal of the email prop from several authentication components and replacing it with useAuthData().
  • Minor formatting and syntax fixes in UI components (e.g., HTML entity corrections).

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/shared/src/contexts/AuthDataContext.tsx New context provider for centralized email state management.
packages/shared/src/components/auth/AuthOptions.tsx Updated to wrap content with AuthDataProvider and use email state from context.
packages/shared/src/components/auth/RegistrationForm.tsx Removed email prop and switched to context usage.
packages/shared/src/components/auth/ForgotPasswordForm.tsx Updated to use context for email state and set email on send.
packages/shared/src/components/auth/CodeVerificationForm.tsx Replaced initialEmail prop with context state for email.
packages/shared/src/components/auth/EmailCodeVerification.tsx Removed email prop and now accesses email state via context.
Comments suppressed due to low confidence (1)

packages/shared/src/components/auth/AuthOptions.tsx:190

  • [nitpick] Consider renaming 'AuthOptionsWithContext' to a more descriptive name (e.g., 'AuthOptionsProviderWrapper') to clearly convey its role in wrapping the auth options with the AuthDataContext.
function AuthOptionsWithContext() {

Co-authored-by: Ole-Martin Bratteng <1681525+omBratteng@users.noreply.github.com>

logEvent({
event_name: AuthEventNames.SignupSuccessfully,
function AuthOptionsWithContext() {
Copy link
Member

Choose a reason for hiding this comment

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

Not even a useCallback or useMemo on this 🙈

@rebelchris rebelchris requested a review from Copilot March 26, 2025 14:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@rebelchris rebelchris requested review from a team, AmarTrebinjac, capJavert, Copilot, ilasw, nensidosari and sshanzel and removed request for a team March 27, 2025 07:55
Copy link
Contributor

Copilot AI left a 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 introduces a new context provider (AuthDataProvider) to manage email state consistently during onboarding and related authentication flows. Key changes include:

  • Adding the AuthDataContext to encapsulate email state and its updater.
  • Refactoring authentication and onboarding components (ForgotPasswordForm, RegistrationForm, CodeVerificationForm, EmailCodeVerification) to use the context instead of receiving the email value directly.
  • Updating tests and page components (e.g. verification.tsx, onboarding.tsx) to wrap the affected components with AuthDataProvider.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/shared/src/contexts/AuthDataContext.tsx New provider for managing email state across authentication flows.
packages/shared/src/components/auth/common.tsx Added AuthDisplay enum and updated type imports for consistency.
packages/shared/src/components/auth/ForgotPasswordForm.spec.tsx Updated tests to wrap ForgotPasswordForm with AuthDataProvider.
packages/shared/src/components/auth/RegistrationForm.tsx Removed redundant email prop and now sourcing from AuthDataContext.
packages/webapp/pages/verification.tsx Updated page to wrap EmailCodeVerification with AuthDataProvider.
packages/shared/src/components/auth/ForgotPasswordForm.tsx Refactored to use context-provided email and update it via setEmail().
packages/shared/src/components/auth/CodeVerificationForm.tsx Updated to source email from AuthDataContext.
packages/shared/src/components/auth/EmailCodeVerification.tsx Simplified by removing email prop, now using useAuthData().
packages/shared/src/components/onboarding/OnboardingHeader.tsx Updated import for AuthDisplay to the new common module.
packages/webapp/pages/onboarding.tsx Updated imports to use auth/common and remove deprecated props.
packages/shared/src/components/auth/AuthenticationBanner.tsx Updated AuthOptions and AuthDisplay imports for consistency.

@rebelchris rebelchris requested a review from idoshamun April 1, 2025 12:52
Copy link
Contributor

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Looks amazing! I'm assuming the AuthOptionsInner is basically just the AuthOptions without any change, except maybe the email management.

@rebelchris
Copy link
Contributor Author

@sshanzel correct :) just needed to wrap to get the hook in there.

@rebelchris rebelchris enabled auto-merge (squash) April 3, 2025 06:26
@rebelchris rebelchris disabled auto-merge April 3, 2025 06:33
@rebelchris rebelchris merged commit b2c3362 into main Apr 3, 2025
8 of 10 checks passed
@rebelchris rebelchris deleted the vibe-auth-context branch April 3, 2025 06:33
@vercel vercel bot temporarily deployed to Preview – daily-webapp April 3, 2025 06:41 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants