Show the device code in the welcome flow too - #41
guys-inc-ops[bot] wants to merge 1 commit into
Conversation
3.5.1 fixed the re-authentication dialog. First-run sign-in to GitHub.com is still broken, and it is the path most new users take. There are three sign-in surfaces, not two. Welcome renders Start for dotcom sign-in, and Start was never given the sign-in state at all - only a loadingBrowserAuth boolean. So it showed a spinner and the pre-device-flow message about the browser redirecting back, which no longer happens, and the code had nowhere to appear. The device flow work in #26 wired the code into AuthenticationForm, which on the welcome path is reached only when signing in to Enterprise. Confirmed from a user's log on 3.5.1: [Welcome] advancing to step: SignInToDotComWithBrowser [SignInStore] initializing OAuth device flow [main] opening in browser: https://github.com/login/device and then nothing, because the request had succeeded and the result was rendered by nobody. Counting the markup in the built renderer is what distinguishes these: 3.5.0 had it once, 3.5.1 twice, and with this change three times - one per surface. That count is the check worth keeping.
![clippy-qa[bot]](https://avatars.githubusercontent.com/in/4024528?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clippy reviewed this pull request
The PR adds device-flow verification display to the Welcome flow's Start step. Three real issues were found: the verification URL is rendered as inert styled text (not clickable), the aria-describedby attribute points at an element that only exists in one of three render branches, and TypeScript's control-flow analysis cannot narrow signInState through the extracted boolean authenticating, making signInState.deviceFlow a type-unsafe access. Two nits round out the findings.
5 findings — 3 blocking · 2 nit · 0 pre-existing
1 finding could not be anchored to the diff, so they are here instead:
app/src/ui/welcome/start.tsx:42—aria-describedby="start-description"is a dangling reference whenever deviceFlow or loadingBrowserAuth is active The<section>at line 39 unconditionally carriesaria-describedby="start-description". The element withid="start-description"(line 138) is only emitted in the third branch ofrenderIntroOrCode()— the default case when neitherdeviceFlownorloadingBrowserAuthis truthy. In the device-flow branch (lines 118-131) and the loading branch (lines 133-135), no element with that id is rendered. Screen readers resolve the attribute at read time: when the device code is on screen or the redirect message is shown,aria-describedbypoints at a non-existent node and the description is silently dropped. Fix: either removearia-describedbyfrom the<section>and put it on the individual content elements, or ensure the referenced element is always present.
Requesting changes — there are findings to address. Address each one and mark its thread Resolved (reply first if you think a finding is wrong — a resolved thread is a decision, not a dismissal). Push, or comment /review, and I will review the new commits.
clippy-review: verdict=changes event=REQUEST_CHANGES normal=3 nit=2 pre_existing=0 inline=4 body=1 unresolved=0 head=8095a06
| return ( | ||
| <> | ||
| <p className="device-flow-instructions"> | ||
| Enter this code at <Ref>{deviceFlow.verificationURI}</Ref> to finish |
There was a problem hiding this comment.
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.
| // 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.
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.
| 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.
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.
| private renderIntroOrCode() { | ||
| const { loadingBrowserAuth, deviceFlow } = this.props | ||
|
|
||
| if (deviceFlow) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Clippy reviewed this pull request
Reviewed two files — welcome.tsx and start.tsx — adding device-flow code display to the Welcome screen. Two blocking issues found: a TypeScript type error from accessing signInState.deviceFlow through a boolean variable that loses type narrowing, and an accessibility regression where aria-describedby points to an element that isn't rendered while device flow is active. Two nits about a non-clickable URL and duplicated markup.
4 findings — 2 blocking · 2 nit · 0 pre-existing
1 finding could not be anchored to the diff, so they are here instead:
app/src/ui/welcome/start.tsx:42—aria-describedby="start-description"dangling when device flow or loading is active The<section>always carriesaria-describedby="start-description"(line 42), but the element withid="start-description"is only rendered by the third branch ofrenderIntroOrCode()(line 138) — the default "intro" case. WhendeviceFlowis truthy (lines 118–130) orloadingBrowserAuthis true (line 134), neither branch emits thatid. Before this change, theidparagraph was always rendered unconditionally. Screen readers will follow the broken reference and announce nothing, silently dropping the accessible description of the section during the entire authentication phase. Fix: moveid="start-description"onto the<section>itself, or emit it on a wrapper that is always present.
Requesting changes — there are findings to address. Address each one and mark its thread Resolved (reply first if you think a finding is wrong — a resolved thread is a decision, not a dismissal). Push, or comment /review, and I will review the new commits.
clippy-review: verdict=changes event=REQUEST_CHANGES normal=2 nit=2 pre_existing=0 inline=3 body=1 unresolved=0 head=8095a06
| // 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.
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.
| return ( | ||
| <> | ||
| <p className="device-flow-instructions"> | ||
| Enter this code at <Ref>{deviceFlow.verificationURI}</Ref> to finish |
There was a problem hiding this comment.
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.)
| if (deviceFlow) { | ||
| return ( | ||
| <> | ||
| <p className="device-flow-instructions"> | ||
| Enter this code at <Ref>{deviceFlow.verificationURI}</Ref> to finish | ||
| signing in. Keep this window open — it'll continue automatically | ||
| once you approve. | ||
| </p> | ||
| <div className="device-flow-user-code" role="status"> | ||
| {deviceFlow.userCode} | ||
| </div> | ||
| </> | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Clippy reviewed this pull request
The PR adds a device-flow verification code UI to the Welcome start screen. Two issues: the aria-describedby ARIA attribute on the section element becomes a dangling reference when the device-flow state is active (new regression), and the device-flow JSX in renderIntroOrCode is a verbatim copy of AuthenticationForm.renderDeviceFlow already in the codebase.
2 findings — 1 blocking · 1 nit · 0 pre-existing
1 finding could not be anchored to the diff, so they are here instead:
app/src/ui/welcome/start.tsx:42— aria-describedby references a missing element in device-flow state The<section>always declaresaria-describedby="start-description", butrenderIntroOrCode()only renders the<p id="start-description">element in its third (idle) branch. WhendeviceFlowis truthy — the new state this PR adds — that paragraph is absent from the DOM for the entire duration of the device-flow sign-in, leaving thearia-describedbypointing at a ghost ID. Screen readers will silently skip the section description. TheloadingBrowserAuthbranch had the same pre-existing gap, but that lasts only a brief moment; the device-flow state is the primary UI for this new sign-in path and can last minutes.
Requesting changes — there are findings to address. Address each one and mark its thread Resolved (reply first if you think a finding is wrong — a resolved thread is a decision, not a dismissal). Push, or comment /review, and I will review the new commits.
clippy-review: verdict=changes event=REQUEST_CHANGES normal=1 nit=1 pre_existing=0 inline=1 body=1 unresolved=0 head=8095a06
| private renderIntroOrCode() { | ||
| const { loadingBrowserAuth, deviceFlow } = this.props | ||
|
|
||
| if (deviceFlow) { |
There was a problem hiding this comment.
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.
3.5.1 fixed the re-authentication dialog. First-run sign-in to GitHub.com is still broken — and it's the path most new users take. Reported on 3.5.1.
Three surfaces, not two
I said last time there were two sign-in surfaces and one was missed. There are three, and #26 wired the code into the least-used one:
welcome/start.tsxsign-in/sign-in.tsxlib/sign-in.tsx+AuthenticationFormStartwas never handed the sign-in state at all — only aloadingBrowserAuthboolean. So it rendered a spinner andBrowserRedirectMessage, the pre-device-flow text promising the browser will redirect back, which no longer happens. The code had nowhere to appear.From the user's 3.5.1 log:
…and then nothing. The request succeeded; the result was rendered by nobody.
The fix
welcome.tsxpassessignInState.deviceFlowintoStart;Startrenders the code when it exists, falling back to the old copy before sign-in begins. Same markup as the other two surfaces, and the styles already work everywhere since #39 un-scoped them.The check that actually catches this class of bug
Counting the markup in the built renderer, which is the only thing that has reliably distinguished these:
yarn compile:prodexits 0;tsc,eslint,prettierclean.Honest note
This is the second miss on the same bug. Both times I inferred which component rendered a path instead of checking, and both times a user found it before I did. The durable fix is a test that asserts "a sign-in state carrying a device code renders that code" against all three surfaces, so the count can't silently be wrong again — worth doing before the next release rather than after.