Skip to content
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

Prefill workspace invitation email (#7174) #8826

Merged

Conversation

eliasylonen
Copy link
Contributor

Prefill workspace invitation email, fixes #7174

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR implements email prefilling for workspace invitations, replacing the generic sign-in prefill state with a more specific developer default state.

  • Added email prefilling from invitation links in useSignInUpForm.ts by reading URL search params
  • Modified workspace-invitation.service.ts to include email parameter in invitation links
  • Replaced isSignInPrefilledState with isDeveloperDefaultSignInPrefilledState for development/testing purposes
  • Simplified WorkspaceInviteTeam component by removing redundant handleKeyDown handler
  • Updated auth tests to reflect new state management approach

💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!

8 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +81 to +83
const isDeveloperDefaultSignInPrefilled = useRecoilValue(
isDeveloperDefaultSignInPrefilledState,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider using object destructuring with useRecoilValue to improve readability and reduce line count

Comment on lines 23 to 24
const setisDeveloperDefaultSignInPrefilled = useSetRecoilState(
isDeveloperDefaultSignInPrefilledState,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: camelCase naming convention not followed for 'setisDeveloperDefaultSignInPrefilled' - first word should be capitalized

Comment on lines 231 to 233
link.searchParams.set('inviteToken', invitation.value.appToken.value);
link.searchParams.set('email', invitation.value.email);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Consider URL encoding the email parameter value to prevent potential URL injection issues

Copy link
Contributor

@AMoreaux AMoreaux left a comment

Choose a reason for hiding this comment

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

Nice UX improvement! 🙏
I noticed a small typo, and as I mentioned in the issue, the last point remains during my test."

@@ -20,7 +20,9 @@ export const ClientConfigProviderEffect = () => {
const setIsDebugMode = useSetRecoilState(isDebugModeState);
const setIsAnalyticsEnabled = useSetRecoilState(isAnalyticsEnabledState);

const setIsSignInPrefilled = useSetRecoilState(isSignInPrefilledState);
const setisDeveloperDefaultSignInPrefilled = useSetRecoilState(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const setisDeveloperDefaultSignInPrefilled = useSetRecoilState(
const setIsDeveloperDefaultSignInPrefilled = useSetRecoilState(

FelixMalfait and others added 2 commits December 2, 2024 13:02
@FelixMalfait
Copy link
Member

I think the error might have to do with clicking twice on the button? If it's too slow users tend to click twice. Could be a wrong lead. Let's close the PR like this and you can fix the item (3) in another PR @eliasylonen?

@FelixMalfait FelixMalfait merged commit b6701a8 into twentyhq:main Dec 2, 2024
17 checks passed
Copy link

github-actions bot commented Dec 2, 2024

Thanks @eliasylonen for your contribution!
This marks your 2nd PR on the repo. You're top 19% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

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.

Follow-ups after new email invite feature
4 participants