-
Notifications
You must be signed in to change notification settings - Fork 0
Show the device code in the welcome flow too #41
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
base: linux
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import * as octicons from '../octicons/octicons.generated' | |
| import { Button } from '../lib/button' | ||
| import { Loading } from '../lib/loading' | ||
| import { BrowserRedirectMessage } from '../lib/authentication-form' | ||
| import { Ref } from '../lib/ref' | ||
| import { SamplesURL } from '../../lib/stats' | ||
|
|
||
| /** | ||
|
|
@@ -20,6 +21,15 @@ interface IStartProps { | |
| readonly advance: (step: WelcomeStep) => void | ||
| readonly dispatcher: Dispatcher | ||
| readonly loadingBrowserAuth: boolean | ||
|
|
||
| /** | ||
| * The device flow verification, once GitHub has issued one. Present means | ||
| * there is a code on screen for the user to type into their browser. | ||
| */ | ||
| readonly deviceFlow?: { | ||
| readonly userCode: string | ||
| readonly verificationURI: string | ||
| } | ||
| } | ||
|
|
||
| /** The first step of the Welcome flow. */ | ||
|
|
@@ -35,17 +45,7 @@ export class Start extends React.Component<IStartProps, {}> { | |
| <h1 className="welcome-title"> | ||
| Welcome to <span>GitHub Desktop</span> | ||
| </h1> | ||
| {!this.props.loadingBrowserAuth ? ( | ||
| <> | ||
| <p id="start-description" className="welcome-text"> | ||
| GitHub Desktop is a seamless way to contribute to projects on | ||
| GitHub and GitHub Enterprise. Sign in below to get started with | ||
| your existing projects. | ||
| </p> | ||
| </> | ||
| ) : ( | ||
| <p>{BrowserRedirectMessage}</p> | ||
| )} | ||
| {this.renderIntroOrCode()} | ||
|
|
||
| <div className="welcome-main-buttons"> | ||
| <Button | ||
|
|
@@ -107,6 +107,42 @@ export class Start extends React.Component<IStartProps, {}> { | |
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Before sign in starts, explain what this is. Once a device code exists, | ||
| * show it - it is the only thing standing between the user and being signed | ||
| * in, and it appears nowhere else in this flow. | ||
| */ | ||
| private renderIntroOrCode() { | ||
| const { loadingBrowserAuth, deviceFlow } = this.props | ||
|
|
||
| if (deviceFlow) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Device flow UI block duplicated verbatim across three files The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Device-flow JSX duplicates authentication-form.tsx:renderDeviceFlow verbatim The new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: renderIntroOrCode duplicates AuthenticationForm.renderDeviceFlow verbatim The ...{deviceFlow.verificationURI}... + block in renderIntroOrCode() is character-for-character identical to AuthenticationForm.renderDeviceFlow() (authentication-form.tsx:52-68). This is now the third copy in the codebase (also in sign-in.tsx). Instruction text or markup changes must be applied in all three places. The fix is to extract a shared component, or render here — which the enterprise sign-in step already uses as the shared layer.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Device-flow JSX block is duplicated verbatim from authentication-form.tsx and sign-in.tsx The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Device-flow instruction markup duplicated verbatim across three components The |
||
| return ( | ||
| <> | ||
| <p className="device-flow-instructions"> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Device-flow UI JSX duplicated verbatim for the third time
|
||
| Enter this code at <Ref>{deviceFlow.verificationURI}</Ref> to finish | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. verificationURI rendered as inert styled text instead of a clickable link
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: renders verificationURI as non-clickable monospace text Ref renders as (ref.tsx) — a styled inline element with no href. The user is told 'Enter this code at [url]' but cannot click the URL; they must type it manually. This bug is pre-existing in authentication-form.tsx:59 (which this code was copied from), but fixing it here would be straightforward: wrap with a LinkButton or open via shell.openExternal on click. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: verificationURI displayed as inert monospace text rather than a clickable link
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verification URI rendered in non-interactive Ref instead of a clickable link
|
||
| signing in. Keep this window open — it'll continue automatically | ||
| once you approve. | ||
| </p> | ||
| <div className="device-flow-user-code" role="status"> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: role="status" live region announces bare device code with no label
|
||
| {deviceFlow.userCode} | ||
| </div> | ||
| </> | ||
| ) | ||
|
Comment on lines
+118
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Device-flow display markup duplicated from The |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: role="status" live region covers only the user code, not the surrounding instructions
|
||
|
|
||
| if (loadingBrowserAuth) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pre-existing: aria-describedby target also missing when loadingBrowserAuth is true Pre-existing before this diff: the old inline ternary also omitted |
||
| return <p>{BrowserRedirectMessage}</p> | ||
| } | ||
|
|
||
| return ( | ||
| <p id="start-description" className="welcome-text"> | ||
| GitHub Desktop is a seamless way to contribute to projects on GitHub and | ||
| GitHub Enterprise. Sign in below to get started with your existing | ||
| projects. | ||
| </p> | ||
| ) | ||
| } | ||
|
|
||
| private signInWithBrowser = (event?: React.MouseEvent<HTMLButtonElement>) => { | ||
| if (event) { | ||
| event.preventDefault() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -137,17 +137,25 @@ export class Welcome extends React.Component<IWelcomeProps, IWelcomeState> { | |
| switch (step) { | ||
| case WelcomeStep.Start: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: const declarations inside a fall-through switch case without enclosing braces
|
||
| case WelcomeStep.SignInToDotComWithBrowser: | ||
| const loadingBrowserAuth = | ||
| const authenticating = | ||
| step === WelcomeStep.SignInToDotComWithBrowser && | ||
| signInState !== null && | ||
| signInState.kind === SignInStep.Authentication && | ||
| signInState.loading | ||
| signInState.kind === SignInStep.Authentication | ||
|
|
||
| const loadingBrowserAuth = authenticating && signInState.loading | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript loses signInState null-narrowing through the authenticating boolean signInState is typed SignInState | null. The null check is folded into the boolean authenticating, but TypeScript's control-flow narrowing does not propagate through an intermediate const assignment. At line 145 (signInState.loading) and line 151 (signInState.deviceFlow), TypeScript still sees signInState as SignInState | null. With strict: true in tsconfig this is a compile error: 'Object is possibly null'. The original single-expression form threaded all guards inline, preserving the narrowing — splitting across three consts breaks it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. signInState.loading accessed without type-predicate narrowing — TypeScript type-safety gap
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type error: signInState.loading accessed without narrowing through authenticating boolean The old single-expression |
||
|
|
||
| // Sign in is the OAuth device flow, so there is a code the user has to | ||
| // type into their browser. This is the only component rendered for | ||
| // dotcom sign-in, so if it is not handed the code there is nowhere for | ||
| // the code to appear. | ||
| const deviceFlow = authenticating ? signInState.deviceFlow : undefined | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript cannot narrow
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type error:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. signInState.deviceFlow is not a property of SignInState — type error under strictNullChecks deviceFlow is declared only on IAuthenticationState (sign-in-store.ts:128), not on ISignInState or any other union member (IEndpointEntryState, ISuccessState, etc.). Even if the null issue at line 145 were fixed, line 151 is still a type error: 'Property deviceFlow does not exist on type SignInState'. TypeScript requires the kind === SignInStep.Authentication narrowing to be visible at the point of access, not captured in a boolean. Fix: use an if (authenticating) block or cast to IAuthenticationState after re-applying the kind check inline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type error: signInState.deviceFlow accessed without narrowing through authenticating boolean
|
||
|
|
||
| return ( | ||
| <Start | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Device-flow request failure is stored but never surfaced to the user When |
||
| advance={this.advanceToStep} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cancelBrowserAuth does not stop the device-flow poll loop Start.cancelBrowserAuth (start.tsx:155) only calls this.props.advance(WelcomeStep.Start). It never calls dispatcher.resetSignInState() or any equivalent. The sign-in store's cancelAuthentication/reset() method (which bumps deviceFlowAttempt to make isCancelled() return true) is never invoked. The background pollForDeviceFlowAccessToken loop therefore continues running after the user clicks Cancel. If the user later approves the device in their browser, emitAuthenticate fires, welcome.tsx's advanceOnSuccessfulSignIn triggers on the next prop update, and the user is silently signed in and advanced to ConfigureGit — despite having explicitly cancelled. |
||
| dispatcher={this.props.dispatcher} | ||
| loadingBrowserAuth={loadingBrowserAuth} | ||
| deviceFlow={deviceFlow} | ||
| /> | ||
| ) | ||
|
|
||
|
|
||
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.
Device-flow auth failures are silently swallowed — user sees the intro screen with no feedback
When device-flow polling fails (network error, expired code), the sign-in store sets
error,loading: false, and clearsdeviceFlow, but keepskind === SignInStep.Authentication. Inwelcome.tsx,authenticatingremains true,loadingBrowserAuthbecomes false, anddeviceFlowbecomes undefined.renderIntroOrCode()finds no matching branch and falls through to the default intro paragraph.IStartPropshas noerrorfield andwelcome.tsxnever passessignInState.errorto<Start>. The user sees the 'GitHub Desktop is a seamless way…' text with no indication that sign-in failed and no way to retry.