Skip to content
Merged
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
2 changes: 1 addition & 1 deletion .githooks/commit-msg
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ cat >&2 <<'EOF'

type one of: feat fix chore docs refactor test perf style ci build revert
scope optional, lowercase kebab-case in parens
subject lowercase start, no trailing period, <=72 chars total on line 1
subject lowercase start, no trailing period, <=72 chars (the text after the colon)
breaking change: add ! after type/scope, e.g. feat(api)!: drop v1

Bypass (discouraged): git commit --no-verify
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/cut-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ jobs:
CUR=$(grep -oE 'marketingVersion = "[^"]+"' Project.swift | head -1 | sed 's/.*"\(.*\)"/\1/')
BUILD=$(grep -oE 'buildNumber = "[^"]+"' Project.swift | head -1 | sed 's/.*"\(.*\)"/\1/')
if [[ -z "$CUR" ]]; then echo "::error::no marketingVersion in Project.swift"; exit 1; fi
if [[ ! "$CUR" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
echo "::error::marketingVersion '$CUR' is not a 3-part numeric version (MAJOR.MINOR.PATCH) — cannot bump"; exit 1
fi
IFS=. read -r MA MI PA <<< "$CUR"
case "$BUMP" in
none) NEW="$CUR" ;;
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -307,5 +307,6 @@ jobs:
git config user.email "release-bot@users.noreply.github.com"
git config user.name "gbar release bot"
git add Casks/gbar.rb
git commit -m "gbar ${VERSION}" || { echo "no cask change"; exit 0; }
git diff --cached --quiet && { echo "no cask change"; exit 0; }
git commit -m "gbar ${VERSION}"
git push
2 changes: 1 addition & 1 deletion .github/workflows/require-milestone.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: require milestone

on:
pull_request:
types: [opened, edited, reopened, synchronize, ready_for_review, demilestoned]
types: [opened, edited, reopened, synchronize, ready_for_review, milestoned, demilestoned]

permissions:
pull-requests: read
Expand Down
6 changes: 6 additions & 0 deletions gbar/Sources/Auth/AuthErrorCopy.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ enum AuthErrorCopy {
"GitHub rate-limited the request. Try again in a little while."
case 404:
"GitHub couldn't find that endpoint. Check the API base URL for your host under Advanced."
case 405:
"GitHub won't merge this PR — it's likely blocked by required checks or reviews."
case 409:
"This PR changed on GitHub (new commits or a merge conflict). Refresh and try again."
case 422:
"GitHub rejected the request. The PR may already be merged, closed, or missing required approvals."
case 500...599:
"GitHub is having trouble right now. Try again in a moment."
default:
Expand Down
21 changes: 18 additions & 3 deletions gbar/Sources/Auth/DeviceFlowClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ actor DeviceFlowClient {
/// Step 2: poll until the user authorizes (or the code expires). Returns the token.
func pollForToken(_ code: DeviceCode) async throws -> String {
let url = webBaseURL.appendingPathComponent("login/oauth/access_token")
var waitNanos = UInt64(max(code.interval, 1)) * 1_000_000_000
var waitNanos = Self.backoffNanos(code.interval)
let deadline = ContinuousClock.now.advanced(by: .seconds(code.expiresIn))

while ContinuousClock.now < deadline {
Expand All @@ -72,15 +72,24 @@ actor DeviceFlowClient {
"device_code": code.deviceCode,
"grant_type": "urn:ietf:params:oauth:grant-type:device_code",
]
let (data, _) = try await post(url, form: body)
let (data, response) = try await post(url, form: body)
// A transient 5xx or a 429 (or a proxy's non-JSON body behind either) would fail the
// decode and abort the whole sign-in — keep polling instead. The device-flow error
// states (`authorization_pending`/`slow_down`/…) arrive as JSON on 200 or 4xx, so decode
// everything else.
if let http = response as? HTTPURLResponse,
http.statusCode == 429 || (500...599).contains(http.statusCode)
{
continue
}
let decoded = try jsonDecoder().decode(TokenResponse.self, from: data)
if let token = decoded.accessToken { return token }
switch decoded.error {
case "authorization_pending": continue
// GitHub's `slow_down` carries the *new* required interval (already increased), not a
// delta — replace, don't accumulate, or repeated slow-downs compound and burn the
// expiry window.
case "slow_down": waitNanos = UInt64(max(decoded.interval ?? 5, 1)) * 1_000_000_000
case "slow_down": waitNanos = Self.backoffNanos(decoded.interval ?? 5)
case "expired_token": throw DeviceFlowError.expiredToken
case "access_denied": throw DeviceFlowError.accessDenied
case let other?: throw DeviceFlowError.unexpected(other)
Expand All @@ -90,6 +99,12 @@ actor DeviceFlowClient {
throw DeviceFlowError.expiredToken
}

/// Poll back-off in nanoseconds, clamped to `[1, 60]` seconds so a hostile or garbage
/// host-supplied interval can't overflow the `UInt64` multiply (or stall the loop for hours).
private static func backoffNanos(_ interval: Int) -> UInt64 {
UInt64(min(max(interval, 1), 60)) * 1_000_000_000
}

private func post(_ url: URL, form: [String: String]) async throws -> (Data, URLResponse) {
var request = URLRequest(url: url)
request.httpMethod = "POST"
Expand Down
29 changes: 27 additions & 2 deletions gbar/Sources/GitHub/GitHubAPI.swift
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,12 @@ struct GitHubClient: GitHubAPI {
guard Self.hasNextPage(response) else { return all }
page += 1
}
// Hit the cap with a `rel="next"` still advertised: we keep the OLDEST pages, so the
// viewer's newest verdict may be missing and the derived gate can read stale. Rare (needs
// >`reviewsPageCap * reviewsPerPage` reviews), but log it — same as `starredRepos`.
let cap = Self.reviewsPageCap * Self.reviewsPerPage
Log.network
.warning("reviews for \(repo, privacy: .public)#\(number) truncated at \(cap, privacy: .public)")
return all
}

Expand Down Expand Up @@ -352,6 +358,11 @@ struct GitHubClient: GitHubAPI {
throw ClientError.badURL
}
components.queryItems = queryItems
// `URLComponents` leaves a literal `+` unescaped in query values, so GitHub reads it as a
// space — a saved search like `c++` would silently become `c `. Re-encode `+` as `%2B`.
if let query = components.percentEncodedQuery {
components.percentEncodedQuery = query.replacingOccurrences(of: "+", with: "%2B")
}
guard let url = components.url else { throw ClientError.badURL }
// Never send the bearer token over cleartext: reject a non-https base URL (e.g. a
// misconfigured Enterprise host pasted as `http://…`) rather than leaking credentials.
Expand Down Expand Up @@ -421,15 +432,29 @@ struct GitHubClient: GitHubAPI {
// secondary limit.
if http.statusCode == 403 || http.statusCode == 429 {
let remaining = http.value(forHTTPHeaderField: "X-RateLimit-Remaining")
if remaining == "0" || http.value(forHTTPHeaderField: "Retry-After") != nil {
// A secondary/abuse-limit 403 often carries neither `Remaining: 0` nor `Retry-After` —
// only an `X-RateLimit-Reset` and/or a "secondary rate limit"/"abuse" body. Treat those
// as rate limits too, or the store never backs off and keeps tripping the same limit.
if remaining == "0"
|| http.value(forHTTPHeaderField: "Retry-After") != nil
|| http.value(forHTTPHeaderField: "X-RateLimit-Reset") != nil
|| Self.bodyMentionsRateLimit(data)
{
throw ClientError.rateLimited(until: Self.rateLimitReset(from: http))
}
}
throw ClientError.http(http.statusCode)
}

/// Whether a 403 body reads as a secondary/abuse rate limit rather than a permission failure.
private static func bodyMentionsRateLimit(_ data: Data) -> Bool {
guard let body = String(data: data, encoding: .utf8)?.lowercased() else { return false }
return body.contains("secondary rate limit") || body.contains("abuse")
}

/// When GitHub says access resumes, from `Retry-After` (relative seconds) or the absolute
/// `X-RateLimit-Reset` epoch; nil when neither header is present.
/// `X-RateLimit-Reset` epoch; nil when neither header is present (the store then applies its
/// own ~60s default back-off).
private static func rateLimitReset(from http: HTTPURLResponse) -> Date? {
if let retryAfter = http.value(forHTTPHeaderField: "Retry-After").flatMap(Double.init) {
return Date().addingTimeInterval(retryAfter)
Expand Down
11 changes: 9 additions & 2 deletions gbar/Sources/GitHub/GraphQLPRMapping.swift
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,10 @@ extension GitHubGraphQL.PRNode {
id: databaseId ?? number,
number: number,
title: title ?? "",
state: state.lowercased(),
// REST represents a merged PR as `state == "closed"` + `merged == true`; GraphQL
// splits MERGED into its own state. Fold MERGED back to "closed" so both hydration
// paths produce an identical `detail`, and carry the merged flag separately.
state: state == "MERGED" ? "closed" : state.lowercased(),
htmlURL: url ?? "",
merged: state == "MERGED",
mergeable: Self.mergeableBool(mergeable),
Expand Down Expand Up @@ -230,7 +233,11 @@ extension GitHubGraphQL.ContextNode {
CheckRun(
id: databaseId ?? index,
name: name ?? "check",
status: status?.lowercased() ?? "",
// A missing status maps to "completed", not "" — `CheckRun.ciStatus` treats anything
// other than "completed" as pending, so an absent status paired with a present
// conclusion (a finished run) would misclassify as pending. CheckRun nodes always
// carry a status; this is the correct fallback for the rare null.
status: status?.lowercased() ?? "completed",
conclusion: conclusion?.lowercased(),
startedAt: startedAt,
completedAt: completedAt
Expand Down
5 changes: 4 additions & 1 deletion gbar/Sources/Menu/MenuContentView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,10 @@ struct MenuContentView: View {
private func toggleSearch() {
withAnimation(Motion.respecting(reduceMotion, Motion.spring)) { searchActive.toggle() }
if searchActive {
searchFocused = true
// The SearchField only mounts once `searchActive` flips and its slide-in transition
// settles, so setting @FocusState synchronously here targets a field that doesn't yet
// exist and no-ops. Defer to the next runloop tick (mirrors ApproveComposer.onAppear).
DispatchQueue.main.async { searchFocused = true }
} else {
searchText = ""
}
Expand Down
13 changes: 10 additions & 3 deletions gbar/Sources/Menu/NotificationMapping.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,17 @@ extension GitHubNotification {
let reposRange = apiURL.path.range(of: "/repos/")
else { return nil }

let tail = apiURL.path[reposRange.upperBound...]
.replacingOccurrences(of: "/pulls/", with: "/pull/")
// Rewrite only the resource-type segment (owner/repo/<type>/…), not every "pulls" in the
// path: a string replace over the whole tail also rewrites an owner, repo, or branch that
// happens to be named "pulls". Segment 2 is the resource type — map only it, "pulls"→"pull".
var segments = apiURL.path[reposRange.upperBound...]
.split(separator: "/", omittingEmptySubsequences: false)
.map(String.init)
if segments.count > 2, segments[2] == "pulls" {
segments[2] = "pull"
}
var components = URLComponents(url: AppConfig.webBaseURL(forAPI: apiBaseURL), resolvingAgainstBaseURL: false)
components?.path = "/" + tail
components?.path = "/" + segments.joined(separator: "/")
return components?.url
}
}
20 changes: 13 additions & 7 deletions gbar/Sources/Store/AppStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -412,11 +412,7 @@ final class AppStore {
/// credential for migration on the next refresh.
private func restorePersistedAccounts() {
if let data = defaults.data(forKey: Self.accountsKey) {
do {
accounts = try JSONDecoder().decode([Account].self, from: data)
} catch {
Log.store.error("accounts decode failed: \(error.localizedDescription, privacy: .public)")
}
accounts = Self.decodePersistedAccounts(from: data)
}
if let filter = defaults.string(forKey: Self.accountFilterKey),
accounts.contains(where: { $0.id == filter })
Expand Down Expand Up @@ -566,8 +562,8 @@ extension AppStore {
/// are an N+1 over the PR list, so skipping them while limited avoids digging the hole deeper
/// (the sections/notifications that already loaded still show; feeds rehydrate next poll).
private func kickOffHydration(accountAPIs: [(account: Account, api: GitHubAPI)]) {
if rateLimitedUntil.map({ $0 > Date() }) == true { return }
let apis = Dictionary(uniqueKeysWithValues: accountAPIs.map { ($0.account.id, $0.api) })
guard !hydrationSkippedForRateLimit(apis: apis) else { return }
hydrateChecks(for: sections, apis: apis)
hydrateRepoFeeds(apis: apis)
}
Expand Down Expand Up @@ -700,7 +696,11 @@ extension AppStore {
expiredAccountID = nil
reauthStatus = .idle
startPolling()
await refresh()
// Force: a poll refresh may already be in flight, built from the account list *before* this
// account was appended. A non-force call would coalesce onto it and return, leaving the new
// account's data unloaded until the next poll (or never, with polling off). Supersede it so
// the new account loads now — mirroring `reconnect` (#10).
await refresh(force: true)
}

/// Remove one account: drop its token, metadata, and all of its merged data. If it was the
Expand All @@ -715,6 +715,11 @@ extension AppStore {
checksGeneration += 1
repoFeedsTask?.cancel()
repoFeedsGeneration += 1
// A post-approve merge-readiness poll captured an `AccountItem` and API client that may
// belong to the account being removed; cancel it so it stops polling with (and holding)
// removed-account state. It nils itself only on normal completion, so nil it here.
mergeReadinessTask?.cancel()
mergeReadinessTask = nil
sections = sections.map { section in
LoadedSection(
id: section.id,
Expand Down Expand Up @@ -786,6 +791,7 @@ extension AppStore {
hasLoaded = false
sessionExpired = false
expiredAccountID = nil
rateLimitedUntil = nil
reauthStatus = .idle
lastErrorMessage = nil
// Reset every notification baseline so the next sign-in re-seeds silently instead of
Expand Down
36 changes: 36 additions & 0 deletions gbar/Sources/Store/AppStoreAccountDecode.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import Foundation

/// Lenient decoding of the persisted accounts blob. Split out of `AppStore` (which is at its
/// SwiftLint `file_length` budget) — the logic is pure and static, so it composes cleanly here.
extension AppStore {
/// Decode the persisted accounts blob **leniently**: skip any malformed element instead of
/// dropping the whole list. A strict `[Account].self` decode fails the entire array on one bad
/// element (e.g. a field a newer build added, or a corrupt entry), silently signing the user
/// out of every account and orphaning their Keychain tokens. Decoding element-wise keeps the
/// good accounts and logs how many were dropped. Returns empty if the blob isn't a JSON array.
static func decodePersistedAccounts(from data: Data) -> [Account] {
do {
let decoded = try JSONDecoder().decode([FailableDecodable<Account>].self, from: data)
let accounts = decoded.compactMap(\.value)
let dropped = decoded.count - accounts.count
if dropped > 0 {
Log.store.error("accounts decode dropped \(dropped, privacy: .public) malformed element(s)")
}
return accounts
} catch {
Log.store.error("accounts decode failed: \(error.localizedDescription, privacy: .public)")
return []
}
}
}

/// Decodes `T`, capturing a per-element decode failure as `nil` rather than aborting the whole
/// container — so one corrupt element in a persisted array can't drop every sibling.
private struct FailableDecodable<T: Decodable>: Decodable {
let value: T?

init(from decoder: Decoder) throws {
let container = try decoder.singleValueContainer()
value = try? container.decode(T.self)
}
}
Loading
Loading