Skip to content
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

Prioritized updates #19655

Merged
merged 11 commits into from
Dec 5, 2024
5 changes: 4 additions & 1 deletion app/src/lib/release-notes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { getVersion } from '../ui/lib/app-proxy'
import { formatDate } from './format-date'
import { offsetFromNow } from './offset-from'
import { encodePathAsUrl } from './path'
import { getUserAgent } from './http'

// expects a release note entry to contain a header and then some text
// example:
Expand Down Expand Up @@ -102,7 +103,9 @@ export async function getChangeLog(
changelogURL.searchParams.set('limit', limit.toString())
}

const response = await fetch(changelogURL.toString())
const response = await fetch(changelogURL.toString(), {
headers: { 'user-agent': getUserAgent() },
})
if (response.ok) {
const releases: ReadonlyArray<ReleaseMetadata> = await response.json()
return releases
Expand Down
6 changes: 5 additions & 1 deletion app/src/lib/stats/stats-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { isInApplicationFolder } from '../../ui/main-process-proxy'
import { getRendererGUID } from '../get-renderer-guid'
import { ValidNotificationPullRequestReviewState } from '../valid-notification-pull-request-review'
import { useExternalCredentialHelperKey } from '../trampoline/use-external-credential-helper'
import { getUserAgent } from '../http'

type PullRequestReviewStatFieldInfix =
| 'Approved'
Expand Down Expand Up @@ -424,7 +425,10 @@ export interface IStatsStore {
const defaultPostImplementation = (body: Record<string, any>) =>
fetch(StatsEndpoint, {
method: 'POST',
headers: new Headers({ 'Content-Type': 'application/json' }),
headers: {
'Content-Type': 'application/json',
'user-agent': getUserAgent(),
},
body: JSON.stringify(body),
})

Expand Down
4 changes: 4 additions & 0 deletions app/src/main-process/menu/build-test-menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ export function buildTestMenu() {
label: 'Update banner',
click: emit('test-update-banner'),
},
{
label: 'Update banner (priority)',
click: emit('test-prioritized-update-banner'),
},
{
label: `Showcase Update banner`,
click: emit('test-showcase-update-banner'),
Expand Down
1 change: 1 addition & 0 deletions app/src/main-process/menu/menu-event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ const TestMenuEvents = [
'test-undone-banner',
'test-untrusted-server',
'test-update-banner',
'test-prioritized-update-banner',
'test-update-existing-git-lfs-filters',
'test-upstream-already-exists',
] as const
Expand Down
2 changes: 2 additions & 0 deletions app/src/ui/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3130,6 +3130,8 @@ export class App extends React.Component<IAppProps, IAppState> {
isX64ToARM64ImmediateAutoUpdate={
updateStore.state.isX64ToARM64ImmediateAutoUpdate
}
prioritizeUpdate={updateStore.state.prioritizeUpdate}
prioritizeUpdateInfoUrl={updateStore.state.prioritizeUpdateInfoUrl}
onDismissed={this.onUpdateAvailableDismissed}
isUpdateShowcaseVisible={this.state.isUpdateShowcaseVisible}
emoji={this.state.emoji}
Expand Down
5 changes: 4 additions & 1 deletion app/src/ui/banners/banner.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import * as React from 'react'
import { Octicon } from '../octicons'
import * as octicons from '../octicons/octicons.generated'
import classNames from 'classnames'

interface IBannerProps {
readonly id?: string
readonly timeout?: number
readonly dismissable?: boolean
readonly className?: string
readonly onDismissed: () => void
}

Expand All @@ -19,8 +21,9 @@ export class Banner extends React.Component<IBannerProps, {}> {
private dismissalTimeoutId: number | null = null

public render() {
const cn = classNames('banner', this.props.className)
return (
<div id={this.props.id} className="banner" ref={this.banner}>
<div id={this.props.id} className={cn} ref={this.banner}>
<div className="contents">{this.props.children}</div>
{this.renderCloseButton()}
</div>
Expand Down
56 changes: 44 additions & 12 deletions app/src/ui/banners/update-available.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,31 +24,43 @@ interface IUpdateAvailableProps {
readonly isUpdateShowcaseVisible: boolean
readonly emoji: Map<string, Emoji>
readonly onDismissed: () => void
readonly prioritizeUpdate: boolean
readonly prioritizeUpdateInfoUrl: string | undefined
}

/**
* A component which tells the user an update is available and gives them the
* option of moving into the future or being a luddite.
*/
export class UpdateAvailable extends React.Component<
IUpdateAvailableProps,
{}
> {
export class UpdateAvailable extends React.Component<IUpdateAvailableProps> {
public render() {
return (
<Banner id="update-available" onDismissed={this.props.onDismissed}>
{!this.props.isUpdateShowcaseVisible && (
<Octicon
className="download-icon"
symbol={octicons.desktopDownload}
/>
)}

<Banner
id="update-available"
className={this.props.prioritizeUpdate ? 'priority' : undefined}
dismissable={!this.props.prioritizeUpdate}
onDismissed={this.props.onDismissed}
>
{this.renderIcon()}
{this.renderMessage()}
</Banner>
)
}

private renderIcon() {
if (this.props.isUpdateShowcaseVisible) {
return null
}

if (this.props.prioritizeUpdate) {
return <Octicon className="warning-icon" symbol={octicons.alert} />
}

return (
<Octicon className="download-icon" symbol={octicons.desktopDownload} />
)
}

private renderMessage = () => {
if (this.props.isX64ToARM64ImmediateAutoUpdate) {
return (
Expand Down Expand Up @@ -87,6 +99,26 @@ export class UpdateAvailable extends React.Component<
)
}

if (this.props.prioritizeUpdate) {
return (
<span onSubmit={this.updateNow}>
This version of GitHub Desktop is missing{' '}
{this.props.prioritizeUpdateInfoUrl ? (
<LinkButton uri={this.props.prioritizeUpdateInfoUrl}>
important updates
</LinkButton>
) : (
'important updates'
)}
. Please{' '}
<LinkButton onClick={this.updateNow}>
restart GitHub Desktop
</LinkButton>{' '}
now to install pending updates.
</span>
)
}

return (
<span onSubmit={this.updateNow}>
An updated version of GitHub Desktop is available and will be installed
Expand Down
13 changes: 13 additions & 0 deletions app/src/ui/lib/test-ui-components/test-ui-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,11 @@ export function showTestUI(
})
case 'test-update-banner':
return showFakeUpdateBanner({})
case 'test-prioritized-update-banner':
return showFakeUpdateBanner({
isPriority: true,
priorityInfoUrl: 'https://desktop.github.com',
})
case 'test-update-existing-git-lfs-filters':
return dispatcher.showPopup({ type: PopupType.LFSAttributeMismatch })
case 'test-upstream-already-exists':
Expand All @@ -179,6 +184,8 @@ export function showTestUI(
function showFakeUpdateBanner(options: {
isArm64?: boolean
isShowcase?: boolean
isPriority?: boolean
priorityInfoUrl?: string
}) {
updateStore.setIsx64ToARM64ImmediateAutoUpdate(options.isArm64 === true)

Expand All @@ -187,6 +194,12 @@ export function showTestUI(
return
}

if (options.isPriority !== undefined) {
updateStore.setPrioritizeUpdate(options.isPriority)
}

updateStore.setPrioritizeUpdateInfoUrl(options.priorityInfoUrl)

dispatcher.setUpdateBannerVisibility(true)
}

Expand Down
70 changes: 70 additions & 0 deletions app/src/ui/lib/update-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { enableUpdateFromEmulatedX64ToARM64 } from '../../lib/feature-flag'
import { offsetFromNow } from '../../lib/offset-from'
import { gte, SemVer } from 'semver'
import { getVersion } from './app-proxy'
import { getUserAgent } from '../../lib/http'

/** The last version a showcase was seen. */
export const lastShowCaseVersionSeen = 'version-of-last-showcase'
Expand Down Expand Up @@ -50,6 +51,8 @@ export interface IUpdateState {
lastSuccessfulCheck: Date | null
isX64ToARM64ImmediateAutoUpdate: boolean
newReleases: ReadonlyArray<ReleaseSummary> | null
prioritizeUpdate: boolean
prioritizeUpdateInfoUrl: string | undefined
}

/** A store which contains the current state of the auto updater. */
Expand All @@ -62,6 +65,16 @@ class UpdateStore {

/** Is the most recent update check user initiated? */
private userInitiatedUpdate = true
private _prioritizeUpdate = false
private _prioritizeUpdateInfoUrl: string | undefined = undefined

public get prioritizeUpdate() {
return this._prioritizeUpdate
}

public get prioritizeUpdateInfoUrl() {
return this._prioritizeUpdateInfoUrl
}

public constructor() {
const lastSuccessfulCheckTime = getNumber(lastSuccessfulCheckKey, 0)
Expand Down Expand Up @@ -127,6 +140,8 @@ class UpdateStore {
(await isRunningUnderARM64Translation())
this.status = UpdateStatus.UpdateReady
this.emitDidChange()

this.updatePriorityUpdateStatus()
}

/**
Expand Down Expand Up @@ -168,6 +183,8 @@ class UpdateStore {
lastSuccessfulCheck: this.lastSuccessfulCheck,
newReleases: this.newReleases,
isX64ToARM64ImmediateAutoUpdate: this.isX64ToARM64ImmediateAutoUpdate,
prioritizeUpdate: this.prioritizeUpdate,
prioritizeUpdateInfoUrl: this.prioritizeUpdateInfoUrl,
}
}

Expand All @@ -187,6 +204,7 @@ class UpdateStore {
// button to crash the app if in the subsequent check, there is no update
// available anymore due to a disabled update.
if (this.status === UpdateStatus.UpdateReady) {
this.updatePriorityUpdateStatus()
return
}

Expand Down Expand Up @@ -252,6 +270,32 @@ class UpdateStore {
quitAndInstallUpdate()
}

private async updatePriorityUpdateStatus() {
try {
const response = await fetch(await this.getUpdatesUrl(false), {
method: 'HEAD',
headers: { 'user-agent': getUserAgent() },
})

const prioritizeUpdate =
response.headers.get('x-prioritize-update') === 'true'

const prioritizeUpdateInfoUrl =
response.headers.get('x-prioritize-update-info-url') ?? undefined

if (
this._prioritizeUpdate !== prioritizeUpdate ||
this._prioritizeUpdateInfoUrl !== prioritizeUpdateInfoUrl
) {
this._prioritizeUpdate = prioritizeUpdate
this._prioritizeUpdateInfoUrl = prioritizeUpdateInfoUrl
this.emitDidChange()
}
} catch (e) {
log.error('Error updating priority update status', e)
}
}

/**
* Method to determine if we should show an update showcase call to action.
*
Expand Down Expand Up @@ -302,6 +346,32 @@ class UpdateStore {

this.isX64ToARM64ImmediateAutoUpdate = value
}

/** This method has only been added for ease of testing the update banner in
* this state and as such is limite to dev and test environments */
public setPrioritizeUpdate(value: boolean) {
if (
__RELEASE_CHANNEL__ !== 'development' &&
__RELEASE_CHANNEL__ !== 'test'
) {
return
}

this._prioritizeUpdate = value
}

/** This method has only been added for ease of testing the update banner in
* this state and as such is limite to dev and test environments */
public setPrioritizeUpdateInfoUrl(value: string | undefined) {
if (
__RELEASE_CHANNEL__ !== 'development' &&
__RELEASE_CHANNEL__ !== 'test'
) {
return
}

this._prioritizeUpdateInfoUrl = value
}
}

/** The store which contains the current state of the auto updater. */
Expand Down
5 changes: 5 additions & 0 deletions app/styles/_variables.scss
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,11 @@ $overlay-background-color: rgba(0, 0, 0, 0.4);
--dialog-information-color: #{$blue-400};
--dialog-error-color: #{$red};

/** Banner */
--banner-warning-background: #{$yellow-700};
--banner-warning-text-color: var(--text-color);
--banner-warning-link-color: #{$blue-700};
Copy link
Contributor

@tidy-dev tidy-dev Dec 4, 2024

Choose a reason for hiding this comment

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

For accessibility, we need:
link -> text = 3:1
link/text -> background: 4.5:1

Unfortunately, this is giving link:text of 1.5:1 for light mode.

Here is a link checker tool that has inputs for the link, the text, and the background to make verification easier.
https://webaim.org/resources/linkcontrastchecker/

Feel free to fix as you see fit. When I was testing locally, I tried the hidden bidi characters styles (which have been modified recently for accessibility.) These colors were inspired by warning colors of https://primer.style/components/banner. ... which one could argue that maybe we could use there "Critical" banner styles which would be a redish color kind of like dark mode. https://primer.style/foundations/primitives/color has a --bgColor-danger-muted (for background) and --bgColor-danger-emphasis (for icon) -- but would need to be verified for accessibility. (Notes, the icon needs to have 3:1 to background)

  /** Banner */
  --banner-warning-background: #{$yellow-100}; // copied from --file-warning-background-color
  --banner-warning-text-color: var(--text-color);
  --banner-warning-link-color: var(--link-button-color);
  
  /_update-available.css 
    &.priority {
      .octicon {
          color: var(--file-warning-color);
        }
    ...

Showing using same styles as from bidi character warning


/** File warning */
--file-warning-background-color: #{$yellow-100};
--file-warning-color: #{darken($yellow-700, 10%)};
Expand Down
5 changes: 5 additions & 0 deletions app/styles/themes/_dark.scss
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,11 @@ body.theme-dark {
--dialog-information-color: #{$blue-400};
--dialog-error-color: #{$red-600};

/** Banner */
--banner-warning-background: #{darken($orange-900, 20%)};
--banner-warning-text-color: var(--text-color);
--banner-warning-link-color: #4285fb;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good contrast for dark mode. :)

Choose a reason for hiding this comment

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

thank you yes couldnt agree more


/** File warning */
--file-warning-background-color: #{rgba($yellow-900, 0.4)};
--file-warning-color: #{$yellow-700};
Expand Down
12 changes: 11 additions & 1 deletion app/styles/ui/banners/_update-available.scss
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
#update-available {
.download-icon {
.download-icon,
.warning-icon {
margin-right: var(--spacing);
}

.banner-emoji {
display: inline-block;
margin-right: var(--spacing-half);
}

&.priority {
background-color: var(--banner-warning-background);
color: var(--banner-warning-text-color);

a {
color: var(--banner-warning-link-color);
}
}
}
Loading