Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 47 additions & 11 deletions app/src/ui/welcome/start.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'

/**
Expand All @@ -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. */
Expand All @@ -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
Expand Down Expand Up @@ -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() {

Copy link
Copy Markdown

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 clears deviceFlow, but keeps kind === SignInStep.Authentication. In welcome.tsx, authenticating remains true, loadingBrowserAuth becomes false, and deviceFlow becomes undefined. renderIntroOrCode() finds no matching branch and falls through to the default intro paragraph. IStartProps has no error field and welcome.tsx never passes signInState.error to <Start>. The user sees the 'GitHub Desktop is a seamless way…' text with no indication that sign-in failed and no way to retry.

const { loadingBrowserAuth, deviceFlow } = this.props

if (deviceFlow) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: Device flow UI block duplicated verbatim across three files

The <p className="device-flow-instructions"> + <div className="device-flow-user-code" role="status"> block is character-for-character identical in authentication-form.tsx:58-65, sign-in/sign-in.tsx:200-207, and start.tsx:121-129 (class names, role attribute, and copy are the same). AuthenticationForm.renderDeviceFlow() already encapsulates this but is declared private, forcing the other two sites to inline it. Any future change — adding a 'Copy code' button, fixing the live-region issue above, adjusting copy — must be applied to all three independently. Extracting a small shared <DeviceFlowCode userCode verificationURI /> component would eliminate the divergence risk.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: Device-flow JSX duplicates authentication-form.tsx:renderDeviceFlow verbatim

The new deviceFlow branch in renderIntroOrCode() — the <p className="device-flow-instructions"> paragraph and <div className="device-flow-user-code"> — is a character-for-character copy of AuthenticationForm.renderDeviceFlow at app/src/ui/lib/authentication-form.tsx:52-68. Any future change to the copy string, CSS class, or role attribute must be made in two places. The existing helper should be extracted into a standalone function and called from both sites.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 <p className="device-flow-instructions">…<Ref>…</Ref>…</p> + <div className="device-flow-user-code" role="status">…</div> fragment is copy-pasted across authentication-form.tsx, sign-in.tsx, and now start.tsx. Any wording change or style fix applied to one copy won't automatically appear in the others, causing divergence. Extracting a DeviceFlowCode component that accepts { userCode, verificationURI } would reduce to one source of truth.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: Device-flow instruction markup duplicated verbatim across three components

The <p className="device-flow-instructions">Enter this code at…</p> + <div className="device-flow-user-code" role="status"> block now exists identically in app/src/ui/lib/authentication-form.tsx (renderDeviceFlow()), app/src/ui/sign-in/sign-in.tsx, and the new renderIntroOrCode(). A wording change or ARIA fix must be applied to all three independently. start.tsx already imports BrowserRedirectMessage from authentication-form.tsx, so it has a direct path to share the markup — either by delegating to AuthenticationForm or by extracting a DeviceFlowCode component.

return (
<>
<p className="device-flow-instructions">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: Device-flow UI JSX duplicated verbatim for the third time

authentication-form.tsx already has a dedicated renderDeviceFlow() private method (lines 52–68) that produces the exact same .device-flow-instructions paragraph and .device-flow-user-code div with identical CSS classes, instruction text, and role="status". sign-in.tsx duplicated it inline (lines 198–208). This diff adds a third copy instead of extracting a shared helper. Any future change to the copy, CSS class names, or ARIA structure must be applied in all three places or they silently diverge.

Enter this code at <Ref>{deviceFlow.verificationURI}</Ref> to finish

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

verificationURI rendered as inert styled text instead of a clickable link

<Ref> renders as <em class="ref-component"> with no href or click handler — it is designed for branch names and commit SHAs, not URLs. The verificationURI (e.g. https://github.com/login/device) is therefore displayed as plain monospace text the user must type by hand. LinkButton already exists in this file (used for Terms of Service, Privacy Statement, and 'Create your free account' on lines 74-103) and calls shell.openExternal(uri) on click. Concrete failure: a user whose browser window is lost or was blocked can only complete sign-in by manually retyping the URL. The same bug exists verbatim in authentication-form.tsx:59 and sign-in.tsx:201 — this PR propagates it to a third call site.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: verificationURI rendered as inert <Ref> text instead of a navigable link

<Ref> renders as <em className="ref-component"> — a monospace styled span intended for branch names, SHAs, and paths. The verification URI is a URL the user must navigate to; wrapping it in Ref gives it no link affordance. If the auto-opened browser tab is closed, the user cannot click the displayed URL to reopen it and is forced to manually copy-paste it. A <LinkButton> or plain <a href={deviceFlow.verificationURI}> would let them click through. (The same pattern exists in sign-in.tsx and authentication-form.tsx, but this PR adds another instance to a more prominent first-run screen.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: verificationURI displayed as inert monospace text rather than a clickable link

<Ref>{deviceFlow.verificationURI}</Ref> renders as <em class="ref-component">https://github.com/login/device</em> with no href. Users who need to open the URL on a different device or whose browser failed to open automatically must retype the full URL. The existing sign-in dialog has the same limitation (so this is not a regression from the PR), but extending the pattern here makes the welcome-screen device-flow strictly harder to use than it needs to be. A LinkButton or <a href={deviceFlow.verificationURI}> would make it actionable.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verification URI rendered in non-interactive Ref instead of a clickable link

Ref renders <em className="ref-component"> — a styled but non-interactive element. Every other external URL in this file (CreateAccountURL, Terms of Service, Privacy Statement) uses <LinkButton uri={…}>. Displaying deviceFlow.verificationURI (e.g. https://github.com/login/device) as <Ref> means the user cannot click it to reopen the browser if the automatic redirect was blocked or the tab was closed; they must type the URL manually. Fix: wrap the URI in <LinkButton uri={deviceFlow.verificationURI}> consistent with how all other external URLs in the file are rendered.

signing in. Keep this window open — it'll continue automatically
once you approve.
</p>
<div className="device-flow-user-code" role="status">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: role="status" on a div present at initial render will not be announced by most screen readers

role="status" creates an ARIA live region that announces mutations. Most screen readers (NVDA, JAWS, VoiceOver) do not announce live-region content that is already in the DOM at the time the region is first encountered — they only announce subsequent changes. Since userCode is set once and never updated in place, screen reader users may never hear the device code read aloud automatically and would have to navigate to it manually. A more reliable pattern is to insert the element into the DOM after mount (so the live region fires) or to make the code a focusable, labelled element instead.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: role="status" live region announces bare device code with no label

role="status" implies aria-live="polite" and fires when content is set after mount — which is the normal case here, since deviceFlow arrives via a re-render after the component is already on screen. When it fires, the live region contains only the raw code string (e.g. ABCD-1234) with no label telling the user what it is or where to enter it. The instruction paragraph (.device-flow-instructions) is not part of the live region and will not be re-announced. Adding aria-label="Device verification code" to the div, or wrapping both the instructions and the code in a single live region, would give the announced string context. The same pattern exists pre-existing in sign-in.tsx:205.

{deviceFlow.userCode}
</div>
</>
)
Comment on lines +118 to +130

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: Device-flow display markup duplicated from authentication-form.tsx

The <p className="device-flow-instructions"> / <div className="device-flow-user-code"> block in renderIntroOrCode() is a near-copy of AuthenticationForm.renderDeviceFlow() in app/src/ui/lib/authentication-form.tsx (lines 57–68). Any future change to wording, ARIA, or styling must be applied in both places. Consider passing deviceFlow to AuthenticationForm (which already accepts it via IAuthenticationFormProps) rather than re-implementing the display here.

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

<div className="device-flow-user-code" role="status"> announces only the bare code digits when it appears. The instruction text ("Enter this code at … to finish signing in") is in a sibling <p> with no live-region role, so a screen-reader user already past that paragraph will hear the code announced without any context. Wrapping both the instruction paragraph and the code div in a single role="status" container (or aria-live="polite") ensures the full message is announced together.


if (loadingBrowserAuth) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 id="start-description" from the BrowserRedirectMessage branch (it only rendered the ID on the !loadingBrowserAuth side). The new renderIntroOrCode() preserves this behavior at line 133. The diff does not introduce this case but also does not fix it. The ARIA contract between aria-describedby="start-description" on the section and the conditionally rendered paragraph is fragile and undocumented — the description target can disappear any time auth is in progress.

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()
Expand Down
14 changes: 11 additions & 3 deletions app/src/ui/welcome/welcome.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -137,17 +137,25 @@ export class Welcome extends React.Component<IWelcomeProps, IWelcomeState> {
switch (step) {
case WelcomeStep.Start:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.Start: falls through to case WelcomeStep.SignInToDotComWithBrowser:, and three const bindings (authenticating, loadingBrowserAuth, deviceFlow) are declared in the shared switch scope without a wrapping {} block. No other case currently declares a const, so there is no collision today, but any future case that introduces a same-named binding will produce a compile-time 'already been declared' error. Adding { … } braces around the case body is the conventional fix.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

signInState.loading accessed without type-predicate narrowing — TypeScript type-safety gap

authenticating is a plain boolean, not a type predicate, so TypeScript cannot narrow signInState from SignInState | null based on it. Line 145 (const loadingBrowserAuth = authenticating && signInState.loading) and line 151 (authenticating ? signInState.deviceFlow : undefined) both access members of a potentially-null, non-narrowed union. ISuccessState does not extend the base ISignInState (it has no loading property), so accessing .loading on the full union is unsound. Runtime behavior is safe due to short-circuit evaluation, but the types lie. A type predicate — const isAuthenticating = (s: SignInState | null): s is IAuthenticationState => s !== null && s.kind === SignInStep.Authentication — would make both lines type-safe.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 step === … && signInState !== null && signInState.kind === SignInStep.Authentication && signInState.loading let TypeScript progressively narrow signInState to IAuthenticationState before accessing .loading. Splitting it into const authenticating = … loses that narrowing: authenticating is a plain boolean, not a type predicate, so TypeScript still sees signInState as SignInState | null at line 145. Under strict: true, this is a compile error because null has no .loading and ISuccessState (which does not extend ISignInState) also has no .loading. Fix: keep .loading inside the original chain or restructure as a class-narrowed check.


// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TypeScript cannot narrow signInState through the boolean variable authenticating; signInState.deviceFlow is accessed on the un-narrowed union

authenticating is a plain boolean constant, not an inline type predicate. TypeScript's control-flow analysis does not propagate the narrowing stored in it, so at line 151 signInState is still typed as SignInState | null. deviceFlow only exists on IAuthenticationState — accessing it on SignInState | null should produce a type error under --strictNullChecks. The runtime is safe because the ternary short-circuits when authenticating is false, but the static type is wrong. Fix: move the ternary inline — step === WelcomeStep.SignInToDotComWithBrowser && signInState?.kind === SignInStep.Authentication ? signInState.deviceFlow : undefined — so TypeScript can narrow signInState directly.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Type error: signInState.deviceFlow accessed without TypeScript narrowing

authenticating is a plain boolean, so TypeScript does not carry the narrowing from signInState !== null && signInState.kind === SignInStep.Authentication forward into line 151. At that point signInState is still typed as SignInState | null (the full union). deviceFlow only exists on IAuthenticationState, not on IEndpointEntryState, IExistingAccountWarning, or ISuccessState, so signInState.deviceFlow is a type error. loading on line 145 is fine because it's on the base ISignInState interface. Fix: use an inline type guard or an explicit if-block that narrows signInState directly, e.g. const authState = (step === WelcomeStep.SignInToDotComWithBrowser && signInState?.kind === SignInStep.Authentication) ? signInState : null.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Type error: signInState.deviceFlow accessed without narrowing through authenticating boolean

deviceFlow is declared only on IAuthenticationState; it does not exist on IEndpointEntryState, IExistingAccountWarning, ISuccessState, or null. Because TypeScript cannot propagate narrowing through the intermediate authenticating: boolean, it sees signInState as SignInState | null in the ternary truthy branch. This produces compile errors under strict: true: 'Object is possibly null' and 'Property deviceFlow does not exist on type …'. The correct pattern already used in sign-in.tsx is an inline discriminant check: signInState !== null && signInState.kind === SignInStep.Authentication ? signInState.deviceFlow : undefined.


return (
<Start

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 requestDeviceFlowVerification() throws (network error, rate-limit, etc.) the store's catch block sets error on the Authentication state (sign-in-store.ts ~line 344-359). But welcome.tsx derives only loadingBrowserAuth and deviceFlow from signInState and passes neither error nor any error signal to <Start>. After the failure the store has loading: false and no deviceFlow, so renderIntroOrCode() falls through to the intro paragraph with no indication anything went wrong. The user must try again without knowing the first attempt failed. <Start> should receive and display the error, or welcome.tsx should route to an error step on failure.

advance={this.advanceToStep}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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}
/>
)

Expand Down
Loading