-
Notifications
You must be signed in to change notification settings - Fork 368
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
feat(auth): redirect to targetPath
after login
#1913
feat(auth): redirect to targetPath
after login
#1913
Conversation
@InosRahul is attempting to deploy a commit to the langfuse Team on Vercel. A member of the Team first needs to authorize it. |
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.
thanks for the contribution, this looks good mostly, can you remove this line?
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.
made little changes (cosmetic), one last questions, otherwise seems good to go. thank you!
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.
made additional changes, PR ready to merge but tests keep failing. Can you fix the end to end test?
web/src/__e2e__/auth.spec.ts
Outdated
|
||
await expect(page).toHaveURL(/.*traces/); | ||
|
||
await page |
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.
this test seems to fail, can you fix it?
targetPath
after login
maybe you can just try to navigate to /settings of the project without really needing to press any buttons beyond the login screen, assuming that this is the issue here |
79f3466
to
e983e02
Compare
Fixed now, also cleaned up the commits. |
Looks great, the worker CI test fails on forks, we're about to fix this to make it easier to contribute to langfuse |
Thank you for your contribution! |
What does this PR do?
Fixes #1093
Loom Video: https://www.loom.com/share/a5da2b975d3c42f7a2c7e63cdcc102b6?sid=a5a83a36-3fa3-4f47-a242-b68335ca96bb
Type of change
Mandatory Tasks