Skip to content

Web: add dolt login for local dolt server #404

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

Merged
merged 12 commits into from
Apr 15, 2025
Merged

Conversation

liuliu-dev
Copy link
Contributor

@liuliu-dev liuliu-dev commented Mar 26, 2025

dolt login button in remotes tab:

Screenshot 2025-04-14 at 1 38 03 PM

after adding credentials on dolthub:

Screenshot 2025-04-14 at 1 38 24 PM

@liuliu-dev liuliu-dev requested a review from tbantle22 April 14, 2025 20:29
Copy link
Collaborator

@tbantle22 tbantle22 left a comment

Choose a reason for hiding this comment

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

LGTM!

event.sender.send("login-started", requestId);

let timedOut = false;
const timeoutDuration = 5 * 60 * 1000; // 5 minutes
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like too long maybe? 1 minute might be better?

@@ -245,6 +245,7 @@ export type Mutation = {
deleteBranch: Scalars['Boolean']['output'];
deleteRemote: Scalars['Boolean']['output'];
deleteTag: Scalars['Boolean']['output'];
doltLogin: Scalars['Boolean']['output'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where are the graphql changes for these?

} finally {
setPending(false);
// Cleanup ID reference
if (requestId) setCancelId(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this go at the end of the try block instead? Then you don't need to reassign the requestId variable

Comment on lines 90 to 93
{success.active ? (
<div className={css.successMsg}>
Logged in as {loggedInUser?.username} ({loggedInUser?.email})
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should maybe only show this if loggedInUser is defined too

className={css.loginButton}
disabled={pending}
>
{pending ? "Login..." : "Dolt Login"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{pending ? "Login..." : "Dolt Login"}
{pending ? "Logging in..." : "Dolt Login"}


return (
<QueryHandler
result={{ ...res, data: res.data }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
result={{ ...res, data: res.data }}
result={res}

@liuliu-dev liuliu-dev merged commit d3580e7 into main Apr 15, 2025
1 check passed
@liuliu-dev liuliu-dev deleted the liuliu/dolt-login branch April 15, 2025 18:32
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.

2 participants