Skip to content

Commit 3ddb842

Browse files
committed
Merge branch 'upstream-development'
2 parents b405ea6 + 94eba08 commit 3ddb842

51 files changed

Lines changed: 2718 additions & 411 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/release-notes-instructions.md

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,28 @@ If it was done by an external contributor (not a member of the `desktop` org), a
3737

3838
Do NOT generate entries for:
3939
- CI/CD changes, test-only changes, internal refactoring
40-
- Dependency bumps (unless fixing a security vulnerability)
40+
- Dependency bumps from Dependabot — even if they mention security fixes, these are routine automated updates to build/dev dependencies and are not user-facing
4141
- Build system or developer tooling changes
4242
- Documentation updates
4343
- PRs with `Notes: no-notes` in their body
4444

45-
**Exception:** Security vulnerability fixes should always be included, even if they are dependency updates. Keep them general — do not include CVE numbers. Example:
45+
**Exception:** Updates to **embedded components that ship with the app** (e.g., Git, Git LFS, Git Credential Manager, Electron) should always be included, even when triggered by a security advisory. These are user-facing because they change the software users run. Example:
4646
```
47-
[Fixed] Update embedded Git to address security vulnerability - #4791
47+
[Improved] Update Git for Windows to v2.53.0.windows.3 - #21957
4848
```
4949

50+
## Output Ordering
51+
52+
Entries in the final output **must** be sorted by tag in the order listed in the Tags section above:
53+
54+
1. All `[New]` entries first
55+
2. Then `[Added]`
56+
3. Then `[Fixed]`
57+
4. Then `[Improved]`
58+
5. Then `[Removed]`
59+
60+
Within each tag group, order entries by significance (most impactful first).
61+
5062
## Writing Style
5163

5264
1. **Write for users, not developers** — describe impact on user workflow, not technical process

.github/skills/testing/SKILL.md

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -540,6 +540,17 @@ DESKTOP_E2E_APP_MODE=unpackaged npx playwright test \
540540
app/test/e2e/my-feature.e2e.ts
541541
```
542542

543+
> ⚠️ **Do NOT use `yarn build:dev` for E2E tests.** The development build
544+
> produces an `index.html` that loads the renderer bundle from
545+
> `http://localhost:3000/build/renderer.js` (the webpack dev server). Without
546+
> the dev server running, the React app never mounts and the Playwright
547+
> `waitForFunction` on `desktop-app-container` will time out silently.
548+
>
549+
> Always use `yarn test:e2e:build:unpackaged` (or the combined
550+
> `yarn test:e2e:unpackaged`) which runs a **production** build with
551+
> `DESKTOP_SKIP_PACKAGE=1`. This bundles `renderer.js` directly into `out/`
552+
> so the app is self-contained.
553+
543554
### Handling the welcome flow and macOS dialogs
544555

545556
If your test launches from a fresh state, you will encounter the welcome flow.
@@ -579,6 +590,59 @@ After running E2E tests, collect artifacts from `playwright-videos/`:
579590
Attach the screenshots and video to the Pull Request description or as
580591
comments to show the new UI additions and prove the feature works end-to-end.
581592

593+
#### Faking data or state for ad-hoc E2E screenshots
594+
595+
Some UI features are only visible under specific conditions — for example, a
596+
signed-in GitHub.com account, a populated model list, or an active Copilot
597+
subscription. In ad-hoc E2E tests whose sole purpose is capturing screenshots
598+
for a Pull Request, you can temporarily modify source code to bypass those
599+
conditions and show the full UI.
600+
601+
**Workflow:**
602+
603+
1. Make the minimum temporary changes needed (e.g. hardcode a store property,
604+
inject fake data, force a boolean flag).
605+
2. Rebuild with `yarn test:e2e:build:unpackaged`.
606+
3. Run your ad-hoc E2E spec and capture screenshots/video.
607+
4. **Revert every temporary change** before committing. Verify with
608+
`git diff <file>` that no fake data leaks into the branch.
609+
5. Delete the ad-hoc E2E spec.
610+
611+
**Tips:**
612+
613+
- **Prefer overriding in `getState()`** (in `AppStore`) rather than deep in a
614+
store or API layer. This keeps the blast radius small — a single file, a
615+
few lines — and easy to revert cleanly.
616+
- **Use realistic but obviously fake data.** If you inject model names, use
617+
real-looking IDs and display names so the screenshots read naturally.
618+
- **Guard with `__DEV__` only if you plan to commit the fake data** (not
619+
recommended). For ad-hoc screenshots the code is reverted immediately, so
620+
a plain unconditional override is simpler and avoids issues with `__DEV__`
621+
being `false` in production E2E builds (`RELEASE_CHANNEL=production`).
622+
- **Watch out for tree-shaking.** The E2E build uses `NODE_ENV=production`.
623+
Constants like `__DEV__` are `false` in that mode, so any code guarded by
624+
`if (__DEV__)` will be dead-code-eliminated by webpack. If you need the
625+
fake data to survive the production build, don't guard it.
626+
- **Verify the revert is complete.** After capturing artifacts, run
627+
`git diff -- <files you touched>` and confirm zero output before moving on.
628+
629+
**Example — forcing a populated Copilot model list:**
630+
631+
```ts
632+
// In AppStore.getState(), temporarily replace:
633+
copilotModels: this.copilotModels,
634+
copilotAvailable: this.copilotStore.isAvailable,
635+
636+
// With:
637+
copilotModels:
638+
this.copilotModels !== null && this.copilotModels.length > 0
639+
? this.copilotModels
640+
: fakeModels, // defined as a const above the class
641+
copilotAvailable: true,
642+
```
643+
644+
After screenshots are captured, revert these two lines back to their originals.
645+
582646
---
583647

584648
## Test Helpers Reference

app/package.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"productName": "GitHub Desktop Plus",
66
"bundleID": "com.github.GitHubClient",
77
"companyName": "GitHub, Inc.",
8-
"version": "3.5.8",
8+
"version": "3.5.9-beta1",
99
"main": "./main.js",
1010
"repository": {
1111
"type": "git",
@@ -33,11 +33,12 @@
3333
"codemirror-mode-luau": "^1.0.2",
3434
"codemirror-mode-zig": "^1.0.7",
3535
"compare-versions": "^3.6.0",
36+
"date-fns": "^4.1.0",
3637
"deep-equal": "^1.0.1",
3738
"desktop-notifications": "file:../vendor/desktop-notifications",
3839
"desktop-trampoline": "file:../vendor/desktop-trampoline",
3940
"dexie": "^3.2.3",
40-
"dompurify": "^3.3.2",
41+
"dompurify": "^3.4.0",
4142
"dugite": "^3.2.2",
4243
"electron-window-state": "^5.0.3",
4344
"event-kit": "^2.0.0",

app/src/lib/app-state.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import type { ModelInfo } from '@github/copilot-sdk'
2+
import type { CopilotModelSelections } from './stores/copilot-store'
13
import { Account } from '../models/account'
24
import { CommitIdentity } from '../models/commit-identity'
35
import { IConfigValueOrigin } from './git/config'
@@ -422,6 +424,9 @@ export interface IAppState {
422424
/** Controls the sort order for branch lists in branch-selection views */
423425
readonly branchSortOrder: BranchSortOrder
424426

427+
/** Whether the user prefers absolute dates over relative time in lists */
428+
readonly preferAbsoluteDates: boolean
429+
425430
/**
426431
* Cached repo rulesets. Used to prevent repeatedly querying the same
427432
* rulesets to check their bypass status.
@@ -438,6 +443,21 @@ export interface IAppState {
438443

439444
/** Whether the changes filter is shown */
440445
readonly showChangesFilter: boolean
446+
447+
/**
448+
* Per-feature Copilot model selections. An absent key means the default
449+
* model will be used for that feature.
450+
*/
451+
readonly selectedCopilotModels: CopilotModelSelections
452+
453+
/**
454+
* The list of available Copilot models fetched from the SDK.
455+
* Null when the list has not been fetched yet.
456+
*/
457+
readonly copilotModels: ReadonlyArray<ModelInfo> | null
458+
459+
/** Whether Copilot is available (i.e. a GitHub.com account is signed in). */
460+
readonly copilotAvailable: boolean
441461
}
442462

443463
export enum FoldoutType {

app/src/lib/feature-flag.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,3 +127,5 @@ export function enableAccessibleListToolTips(): boolean {
127127
export const enableHooksEnvironment = () => true
128128

129129
export const enableHooksByDefault = enableBetaFeatures
130+
131+
export const enableFormattingPreferences = enableBetaFeatures

app/src/lib/format-date.ts

Lines changed: 72 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
import { format } from 'date-fns'
2+
import {
3+
getDateFormatPreference,
4+
getTimeFormatPreference,
5+
} from '../models/formatting-preferences'
6+
import { enableFormattingPreferences } from './feature-flag'
17
import mem from 'mem'
28
import QuickLRU from 'quick-lru'
39

@@ -10,12 +16,72 @@ const getDateFormatter = mem(Intl.DateTimeFormat, {
1016
cacheKey: (...args) => JSON.stringify(args),
1117
})
1218

19+
interface IFormatDateOptions {
20+
/** Whether to include the date portion. Defaults to true. */
21+
readonly date?: boolean
22+
/** Whether to include the time portion. Defaults to true. */
23+
readonly time?: boolean
24+
25+
/**
26+
* @deprecated Will be removed in a future release. Temporarily supported for
27+
* backward compatibility with existing code when
28+
* enableFormattingPreferences is disabled. As soon as formatting
29+
* preferences is shipped to production, this option will be
30+
* removed.
31+
*/
32+
readonly dateStyle?: 'full' | 'long' | 'medium' | 'short'
33+
34+
/**
35+
* @deprecated Will be removed in a future release. Temporarily supported for
36+
* backward compatibility with existing code when
37+
* enableFormattingPreferences is disabled. As soon as formatting
38+
* preferences is shipped to production, this option will be
39+
* removed.
40+
*/
41+
readonly timeStyle?: 'full' | 'long' | 'medium' | 'short'
42+
}
43+
1344
/**
14-
* Format a date in en-US locale, customizable with Intl.DateTimeFormatOptions.
45+
* Format a date using the user's preferred date and time format patterns.
1546
*
16-
* See Intl.DateTimeFormat for more information
47+
* By default both date and time are included. Pass `{ date: false }` or
48+
* `{ time: false }` to include only one.
1749
*/
18-
export const formatDate = (date: Date, options: Intl.DateTimeFormatOptions) =>
19-
isNaN(date.valueOf())
20-
? 'Invalid date'
21-
: getDateFormatter('en-US', options).format(date)
50+
export function formatDate(
51+
value: Date,
52+
{ date = true, time = true, dateStyle, timeStyle }: IFormatDateOptions = {}
53+
): string {
54+
if (isNaN(value.valueOf())) {
55+
return 'Invalid date'
56+
}
57+
58+
if (!enableFormattingPreferences()) {
59+
return getDateFormatter('en-US', { dateStyle, timeStyle }).format(value)
60+
}
61+
62+
let formatString: string
63+
64+
if (date && time) {
65+
formatString = `${getDateFormatPreference()} ${getTimeFormatPreference()}`
66+
} else if (date) {
67+
formatString = getDateFormatPreference()
68+
} else if (time) {
69+
formatString = getTimeFormatPreference()
70+
} else {
71+
// If neither date nor time is included, just return an empty string or
72+
// else date-fns will throw because it doesn't know what to do with the
73+
// format string
74+
return ''
75+
}
76+
77+
try {
78+
return format(value, formatString)
79+
} catch (e) {
80+
// In case the user has configured an invalid format pattern, we don't want
81+
// the app to crash, let's fall back to a default format and log the error
82+
// so we can investigate.
83+
log.error(`Error formatting date with format string "${formatString}"`, e)
84+
85+
return value.toISOString()
86+
}
87+
}

app/src/lib/format-number.ts

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
import {
2+
getNumberFormatPreference,
3+
INumberFormat,
4+
} from '../models/formatting-preferences'
5+
import { round } from '../ui/lib/round'
6+
import { enableFormattingPreferences } from './feature-flag'
7+
8+
/**
9+
* Format a number using the given separator configuration.
10+
*
11+
* This is a simple formatter that handles integer and decimal parts with
12+
* configurable separators. It does not use Intl.NumberFormat.
13+
*
14+
* @param value - The number to format
15+
* @param fmt - The number format configuration with thousands and decimal
16+
* separators, defaults to the user's preferred format.
17+
*/
18+
export function formatNumber(value: number, fmt?: INumberFormat): string {
19+
if (!fmt && !enableFormattingPreferences()) {
20+
return value.toString()
21+
}
22+
23+
fmt ??= getNumberFormatPreference()
24+
25+
if (!Number.isFinite(value)) {
26+
return String(value)
27+
}
28+
29+
const isNegative = value < 0
30+
const abs = Math.abs(value)
31+
const [intPart, decPart] = abs.toString().split('.')
32+
33+
// Insert a placeholder character for thousands groupings, then replace with
34+
// the configured separator. The regex matches positions that are followed by
35+
// groups of exactly 3 digits.
36+
const grouped = intPart.replace(/\B(?=(\d{3})+(?!\d))/g, '\x00')
37+
const formattedInt = grouped.replace(/\x00/g, fmt.thousandsSeparator)
38+
39+
const result =
40+
decPart !== undefined
41+
? `${formattedInt}${fmt.decimalSeparator}${decPart}`
42+
: formattedInt
43+
44+
return isNegative ? `-${result}` : result
45+
}
46+
47+
interface ICompactFormatOptions {
48+
/** Number of decimal places to display */
49+
readonly decimals?: number
50+
/**
51+
* The base to use for unit scaling.
52+
* - 1000: SI/decimal units (k, m, b, t or KB, MB, GB)
53+
* - 1024: IEC/binary units (KiB, MiB, GiB)
54+
*/
55+
readonly base?: 1000 | 1024
56+
/**
57+
* Custom unit suffixes to use. If not provided, defaults to:
58+
* - For base 1000: ['', 'k', 'm', 'b', 't']
59+
* - For base 1024: no default (must be provided)
60+
*/
61+
readonly units?: ReadonlyArray<string>
62+
/**
63+
* Whether to add a space between the number and the unit suffix.
64+
* Defaults to false for the shorthand k/m/b/t units.
65+
*/
66+
readonly unitSeparator?: string
67+
68+
readonly numberFormat?: INumberFormat
69+
}
70+
71+
const defaultDecimalUnits = ['', 'k', 'm', 'b', 't']
72+
73+
export function formatCompactNumber(
74+
value: number,
75+
fmt?: ICompactFormatOptions
76+
): string {
77+
if (!fmt && !enableFormattingPreferences()) {
78+
return `${value}`
79+
}
80+
81+
if (!Number.isFinite(value)) {
82+
return `${value}`
83+
}
84+
85+
const abs = Math.abs(value)
86+
const base = fmt?.base ?? 1000
87+
const units = fmt?.units ?? defaultDecimalUnits
88+
const unitSeparator = fmt?.unitSeparator ?? ''
89+
90+
if (abs < base) {
91+
const result = formatNumber(value, fmt?.numberFormat)
92+
// For byte formatting, always show units even for small values
93+
return units[0] ? `${result}${unitSeparator}${units[0]}` : result
94+
}
95+
96+
const unitIx = Math.min(
97+
units.length - 1,
98+
Math.floor(Math.log(abs) / Math.log(base))
99+
)
100+
101+
const scaled = value / Math.pow(base, unitIx)
102+
103+
// If the user didn't provide an explicit number of decimals to use, we'll
104+
// default to 1 decimal for numbers less than 10 and no decimals for numbers
105+
// 10 or greater. This is a common convention for compact number formatting
106+
// that balances precision with brevity.
107+
const decimals = fmt?.decimals ?? (Math.abs(scaled) < 10 ? 1 : 0)
108+
109+
const result = round(scaled, decimals)
110+
return `${formatNumber(result, fmt?.numberFormat)}${unitSeparator}${
111+
units[unitIx]
112+
}`
113+
}

app/src/lib/release-notes.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ export function getReleaseSummary(
7878
return {
7979
latestVersion: latestRelease.version,
8080
datePublished: formatDate(new Date(latestRelease.pub_date), {
81+
time: false,
8182
dateStyle: 'long',
8283
}),
8384
pretext,

0 commit comments

Comments
 (0)