fix: code-review fixes — rate limits, URL hardening, robustness, a11y, coverage - #82
Merged
Conversation
…review verdicts Tier 1 of the code-review fixes: - Map 403/429 with rate-limit headers to ClientError.rateLimited(until:) and back the poll loop off to the reset time instead of hammering into GitHub's secondary limit; skip the request-heavy CI/repo-feed hydration while limited. - Drop a legacy single-token credential that returns 401 on migration so the UI falls back to sign-in instead of a permanent first-load skeleton. - Replace (don't accumulate) the device-flow poll interval on slow_down. - Route notification mark-read / mark-all 401s to the reconnect prompt, matching the quick-action path. - Paginate reviews() to the last page so a >100-review PR's latest verdict is used to gate Approve/Merge. Extract the poll loop into AppStorePolling.swift and fold per-account load failures into a LoadFailures struct to stay within lint budgets.
Tier 2 — URL/scheme hardening: - Reject non-https base URLs in GitHubClient.makeRequest so the bearer token can't go out over cleartext; validate the Enterprise host field at submit. - Add WebLink scheme allowlist and gate every URL-open site (NSWorkspace in NotificationService, openURL in the menu rows/notification rows) on http(s). Tier 3 — robustness: - Close the concurrent force-refresh race (coalesce onto a peer's fresh run). - Tolerant ISO8601 decoding (fractional seconds) so one field can't fail a page. - Nil mergeReadinessTask on completion/sign-out. - Reject '.'/'..' path-traversal segments in normalizedSlug. - Reset stale search state when the filter bar unmounts. - Persist the OAuth client ID only on a successful sign-in; clear the pasted PAT on mode-switch and window close. - justfile: tee the full build log and surface error lines instead of tail -30. Tier 4 — accessibility: - Expose selected/toggle state to VoiceOver on the tab bar, segmented control, and filter chips; hide the decorative UnseenDot when seen. Tier 5 — coverage + CI: - Tests for reconnect (in-place token swap / reentrancy / failure), addAccount invalid + duplicate, maintainer-without-push merge, CI-passed banner, https rejection, WebLink, and normalizedSlug traversal (245 tests, +12). - Cache resolved SwiftPM/Tuist deps in the build & test CI jobs. Extract test hooks (AppStore+TestHooks) and WebLink (Support/) to sibling files to stay within lint budgets. Full Dynamic Type support and a protocol-level API error type are deferred (design decision / YAGNI) — see PR notes.
From three adversarial review agents over the diff: - Data race (real): the ISO8601 fractional-seconds decoder shared a mutable ISO8601DateFormatter across the multi-account TaskGroup — swap to the Sendable value-type Date.ISO8601FormatStyle (no nonisolated(unsafe)). - Gate the device-flow verificationUri (host-returned) through WebLink at both add-account and 401-reconnect sites — the one host URL the allowlist missed. - Keep the first-load skeleton (don't flash an empty state) when a legacy-token migration fails transiently vs. is rejected (401). Coverage the review flagged as missing: - Extract a pure AppStore.pollDelay(...) and test the rate-limit backoff honours the reset and the cadence floor (the headline fix was previously only asserted at the field level). - Make HostField internal and add HostFieldTests (was private → untestable). - Assert hydration is actually skipped while rate-limited (pullRequestCount == 0). - Test ISO8601 fractional/plain/invalid decoding, reviews() page-cap termination, and the rate-limit / 429 AuthErrorCopy branches. - Drop a misleading normalizedSlug case rejected by the arity guard, not the new traversal check. 262 → 256 tests, lint clean, build + smoke green.
Owner
Author
Self-review round (3 adversarial agents over the diff)Ran three review agents against the branch diff; acted on every real finding ( Confirmed & fixed
Coverage the review flagged as missing (added)
Verified-correct by the reviewers (no change needed): the force-refresh race fix, rate-limit backoff clamping, Noted: the SPM half of the CI cache is a near-no-op today (no external SPM deps); the Tuist-cache half still helps. Left in place to future-proof. 256 tests, |
A refresh hydrates every distinct open PR (detail + check-runs + reviews) every poll — an N+1 that exhausted the core 5000/hr limit on a large inbox (~189 core requests/refresh at the 60s cadence). Two mitigations: - ETag revalidation: requests now use `.reloadRevalidatingCacheData` over a private URLCache, so an unchanged resource returns a rate-limit-free 304. This also supersedes the old cache-bypass hack — a changed resource still returns a fresh 200, so approve/merge buttons stay current. - updated_at short-circuit: a PR unchanged since its last hydration skips the detail+reviews refetch and reuses the cached gate, re-reading only its check-runs against the cached head sha (so a CI re-run flip is still caught). Verified live: three consecutive polls consumed ~61 core total (budget flat), with request count falling 193 -> 81 -> 38 as the hydration marks accumulate. Also: harden JSON date decoding coverage (fractional seconds through every required field), pin reviews single-page cost, and log remaining rate budget on 2xx. GraphQL batching to collapse the N+1 structurally is tracked in #83.
Address code-review findings on the hydration N+1 cut: - Never take the checks-only skip for a PR whose Merge button is showing. A base-branch advance can flip its mergeable_state to behind/dirty without bumping the PR's updated_at, so skipping would keep serving a stale Merge button that 405s on click. Mergeable PRs now always refetch (a cheap 304 while genuinely unchanged, a fresh 200 the moment they go behind). - Switch the revalidation URLCache to memory-only (diskCapacity 0) so private PR/review bodies aren't persisted to disk at rest. Steady-state polling stays cheap via in-session 304s; a cold launch pays one full poll. Tests: pin that a mergeable PR is not skipped, and a deterministic checks-only CI-flip-notifies test (fixed updated_at) covering the skip path.
jaylann
added a commit
that referenced
this pull request
Jul 5, 2026
… republish (#84) (#85) ## What Fixes #84 — the post-approval **merge-readiness poll**'s gate write being clobbered by a concurrent **CI hydration wave**'s batch republish, which briefly hides (or wrongly shows) the Merge button after an approval. ## Root cause `prGates` (the per-PR Approve/Merge action gate) is written by two `@MainActor` flows that interleave across `await`: - the hydration wave (`drainChecks`/`fold`/`publishChecks`), which refetches many PRs and republishes gates in batches; - `refreshPRState`, a single-PR gate refetch driven by the post-approve poll. A wave that read a PR's gate *before* the poll's single-key write would republish the whole map and clobber it. The `checksGeneration` guard only catches cross-generation clobbers; this is a same-generation interleave. #82's `.checksOnly` path widened the window. ## Approach — issue-time write-recency The naive fixes fail: "poll always wins" shows a Merge button that 405s when the base advanced; a **commit-time** clock still loses because the wave's full fetch reads its gate *early* (from the PR detail) but folds *late* (after its slow checks/reviews legs), so a wave that observed older state commits last and wrongly wins. Fix: a monotonic **issue-time** clock (`gateWriteClock` + `prGateSeq`). Both writers take a tick when they *issue* their fetch — the wave stamps a per-full-fetch issue seq up front; the poll ticks before its detail request and defers to any concurrently-issued newer write. `publishChecks` merges gates onto the live map per key, applying a wave gate only when its issue tick beats the live one. So the observation that read the **newer server state wins in both clobber directions**, regardless of commit order. Checks-only carry-overs and failed fetches don't compete (they keep the live gate rather than blanking it). `prGateSeq` is pruned alongside `prGates` at every mutation/reset site. A documented, bounded, self-healing residual remains (a whole wave shares one issue instant, so a mid-wave poll beats a late-queued key) — noted in code; per-key stamping would need a main-actor hop per fetch and isn't worth it for a transient gate flip. ## Tests Three deterministic `GatedGitHubAPI` regression tests pinning the interleavings by issue order: - `testMergePollGateWriteSurvivesChecksOnlyRepublish` — checks-only carry-over can't clobber the poll. - `testLaterIssuedMergePollGateBeatsEarlierIssuedWaveFullFetch` — the primary #84 trigger; **verified it fails under commit-time stamping**. - `testLaterIssuedWaveFetchBeatsEarlierMergePollGate` — a fresher wave fetch beats a staler poll write (guards "poll always wins"). `just check` clean, full suite (269 tests) green. Reviewed over three rounds (multi-agent + single-agent); final review passed with no blocking findings. Closes #84
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the fixes from a full-codebase review (5 parallel review agents + direct verification). Organized into the tiers from the review plan; each tier is independently reviewable in the two commits.
245 tests pass (+23 new),
just checkclean, build + launch smoke test green.Tier 1 — behavioral bugs (
eaad5f5)403/429+ rate-limit headers →ClientError.rateLimited(until:); the poll loop backs off to the reset time (skipping the request-heavy CI/repo-feed hydration while limited) instead of hammering into GitHub's secondary limit. Message surfaced to the UI.401on legacy-token migration now drops the dead token so the UI falls back to sign-in instead of a permanent first-load skeleton.slow_down: replace the poll interval instead of accumulating it.markRead/markAllReadroute to the reconnect prompt, matching the quick-action path.reviews()pagination: walk theLinkheader to the last page so a >100-review PR's latest verdict gates Approve/Merge.Tier 2 — URL/scheme hardening (
fbe146b)httpsbase URLs inGitHubClient.makeRequest(no bearer token over cleartext); validate the Enterprise host field at submit with an inline hint.WebLinkscheme allowlist gates every URL-open site (NSWorkspaceinNotificationService,openURLin menu/notification rows) tohttp(s)only — a hostile self-hosted host can't return afile:/javascript:deep link.Tier 3 — robustness
force-refresh race (coalesce onto a peer's fresh run).mergeReadinessTaskon completion/sign-out; reject./..traversal innormalizedSlug; reset stale search state when the filter bar unmounts; persist the OAuth client ID only on success and clear the pasted PAT on mode-switch/close.justfile: tee the full build log + surface error lines instead oftail -30.Tier 4 — accessibility
UnseenDotwhen seen.Tier 5 — coverage + CI
addAccountinvalid + duplicate, maintainer-without-push merge, CI-passed banner, https rejection,WebLink,normalizedSlugtraversal.Deferred (intentionally, want your call)
GitHubAPIError— the store still matchesGitHubClient.ClientError. YAGNI until a secondGitHubAPIbackend exists; low value, non-trivial churn.ClientError.http— would change itsEquatableshape and ripple through many test matches for marginal benefit.🤖 Generated with Claude Code