Skip to content

Consolidate IssueTracker gh calls into one GraphQL query#175

Merged
dhilgaertner merged 4 commits intomainfrom
feature/crow-173-rate-limit-audit
Apr 16, 2026
Merged

Consolidate IssueTracker gh calls into one GraphQL query#175
dhilgaertner merged 4 commits intomainfrom
feature/crow-173-rate-limit-audit

Conversation

@dhilgaertner
Copy link
Copy Markdown
Contributor

Closes #173.

Summary

  • Collapse the per-refresh fan-out in IssueTracker.refresh() into a single aliased gh api graphql call that returns open assigned issues (with project status), viewer PRs (with checks/reviews/mergeable), recent closed issues, review-requested PRs, and the rateLimit block.
  • Rewrite checkSessionPRs, fetchPRStatuses, autoCompleteFinishedSessions, and autoCompleteFinishedReviews as local index lookups over that payload — zero per-session gh calls. Delete checkPRMerged / checkIssueClosed.
  • Add GitHubRateLimit to AppState, proactively skip polls when remaining < 50, parse 403 / Retry-After / X-RateLimit-Reset on failure to suspend until reset, and render a rateLimitWarning banner in SettingsView next to the existing scope-warning banner. Each refresh logs a summary (N gh calls in Xs, GraphQL R/L remaining, resets in Nm).

Steady-state cost: 1 gh call per refresh for a GitHub-only workspace (prior: 30–50).

Test plan

  • make build — clean compile
  • make test — 17/17 pass
  • Run app with ~10 active sessions; verify the [IssueTracker] refresh: N gh calls … log line reports N == 2 in steady state (1 GitHub GraphQL + 1 per GitLab host)
  • Ticket board, review board, and per-session PR status badges render identically to pre-change
  • Merge a PR linked to an active session → the session auto-completes within one refresh, no extra gh pr view calls in the log
  • Force remaining < 50 locally → rateLimitWarning banner shows in Settings, refresh is skipped until reset, banner clears on recovery
  • Revoke read:project scope → existing scope banner still fires; refresh retries without projectItems so the rest of the data still renders

Out of scope (followups)

  • Raising pollInterval / event-driven refresh
  • Replacing gh CLI with a persistent GraphQL client
  • IssueTracker unit tests (needs a shell/Process abstraction that doesn't exist yet)
  • Moving the warning banner from Settings into TicketBoardView / ReviewBoardView

🤖 Generated with Claude Code

@dhilgaertner dhilgaertner requested a review from dgershman as a code owner April 15, 2026 19:01
Copy link
Copy Markdown
Collaborator

@dgershman dgershman left a comment

Choose a reason for hiding this comment

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

Code & Security Review

Critical Issues

None found.

Security Review

Strengths:

  • No secrets or credentials exposed in the codebase
  • Rate-limit handling prevents runaway API usage; proactive threshold (remaining < 50) plus reactive 403/Retry-After parsing is a solid defense-in-depth pattern
  • The retryWithoutProjectItems graceful degradation for INSUFFICIENT_SCOPES keeps the app functional even with reduced token scopes
  • Shell commands use Process with explicit argument arrays — no shell expansion, so no command injection risk
  • GraphQL query variables are passed via -F/-f flags, not interpolated into the query string (except the search queries which use safe @me / type:issue literals)

Concerns:

  • parseResetAt uses .range(of:options:.regularExpression) and then .split(separator: ":") to extract the numeric value, which works but is slightly fragile — it relies on the matched string containing exactly one colon. Not a security issue, but a robustness nit (line 138–154)
  • The closedSinceString() date formatter uses DateFormatter with a hardcoded format string and UTC timezone — correct, but worth a comment that this intentionally differs from the ISO8601DateFormatter used elsewhere

Code Quality

Positive:

  • Massive API call reduction (30–50 → 1–2 per refresh) is a significant improvement
  • Clean separation: ConsolidatedGitHubResponse + parsing functions are well-structured
  • The ViewerPR struct captures all needed fields, enabling applySessionPRLinks, applyPRStatuses, and autoCompleteFinishedSessions to be pure local index lookups
  • retryWithoutProjectItems via string replacement is pragmatic — the alternative (maintaining two query strings) would be worse
  • Review request sorting (newest first) is a good UX touch
  • logRefreshSummary provides useful observability for debugging

Minor items:

  • applyPRStatuses at line 739: Dictionary(uniqueKeysWithValues:) will crash if two PRs share the same URL. This shouldn't happen in practice (GitHub URLs are unique per PR), but Dictionary(viewerPRs.map { ($0.url, $0) }, uniquingKeysWith: { a, _ in a }) would be safer
  • applySessionPRLinks creates a new JSONStore() on every call (line 659). If JSONStore.init is cheap this is fine, but if it reads from disk on init it's worth caching
  • autoCompleteFinishedSessions at line 824: same Dictionary(uniqueKeysWithValues:) pattern — same low-risk crash potential
  • The viewer.pullRequests(first: 100, states: [OPEN, MERGED, CLOSED]) fetches all PR states including merged/closed. For users with many historical PRs, this could return stale data filling the 100-item limit. Consider whether first: 100 is sufficient or if filtering to [OPEN] + a separate recent-merged query would be more reliable for the auto-complete use case
  • retryWithoutProjectItems uses String.replacingOccurrences to strip the projectItems block from the GraphQL query. This works but is sensitive to whitespace — if the query formatting changes, the replacement silently becomes a no-op. A comment warning about this coupling would help future maintainers

Summary Table

Priority Issue
🟡 Yellow Dictionary(uniqueKeysWithValues:) in applyPRStatuses / autoCompleteFinishedSessions could crash on duplicate keys (unlikely but defensive fix is trivial)
🟡 Yellow viewer.pullRequests(first: 100) across all states may miss recent PRs for prolific contributors
🟢 Green retryWithoutProjectItems string replacement is fragile to whitespace changes
🟢 Green JSONStore() instantiated per call in applySessionPRLinks — verify init cost

Recommendation: Approve. This is a well-executed consolidation that dramatically reduces API calls from ~30–50 to 1–2 per refresh cycle. The rate-limit handling is thorough (proactive threshold + reactive 403 parsing + UI banner). The yellow items are low-probability edge cases worth addressing in a follow-up but not blocking.

@dhilgaertner dhilgaertner deleted the feature/crow-173-rate-limit-audit branch April 16, 2026 02:48
@dhilgaertner dhilgaertner restored the feature/crow-173-rate-limit-audit branch April 16, 2026 02:49
@dhilgaertner dhilgaertner reopened this Apr 16, 2026
@dhilgaertner dhilgaertner deleted the feature/crow-173-rate-limit-audit branch April 16, 2026 02:50
@dhilgaertner dhilgaertner restored the feature/crow-173-rate-limit-audit branch April 16, 2026 02:51
@dhilgaertner dhilgaertner reopened this Apr 16, 2026
IssueTracker.refresh() previously fanned out into 30–50 gh invocations per
minute with ~10 active sessions, routinely blowing the GitHub GraphQL
search quota and silently emptying the ticket/review boards until reset.

Replace the per-issue/per-session fan-out with one aliased `gh api graphql`
call that returns open assigned issues + project status, viewer PRs with
checks/reviews/mergeable, recent closed issues, review-requested PRs, and
the `rateLimit` block. Session PR link detection, PR status enrichment,
and auto-complete all now read from that single payload — no per-session
gh calls. Steady-state refresh fires 1 GitHub call plus 1 per unique
GitLab host.

Surface the rate-limit state to AppState (`githubRateLimit`,
`rateLimitWarning`), proactively skip polls when `remaining < 50`, parse
403 / `Retry-After` / `X-RateLimit-Reset` on failure to suspend until
reset, and render a throttled banner in SettingsView alongside the
existing scope-warning banner. Each refresh logs a summary line so the
call count and remaining quota are visible without inspecting traces.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dhilgaertner dhilgaertner force-pushed the feature/crow-173-rate-limit-audit branch from 35b4269 to 5817523 Compare April 16, 2026 03:28
Dustin Hilgaertner and others added 3 commits April 15, 2026 22:41
Read stdout/stderr pipes before calling waitUntilExit() to prevent
deadlock when process output exceeds the 64 KB pipe buffer. The
consolidated GraphQL response is ~86 KB with ~100 PRs, which filled
the buffer and blocked the process from writing — hanging refresh()
indefinitely.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fetch only OPEN viewer PRs (first: 50) instead of all states
(first: 100). Auto-complete infers "done" from absence in the open
set rather than checking for MERGED state. Also reduce
statusCheckRollup contexts from 50→25 and latestReviews from 10→5.

Response size drops from ~86 KB to ~34 KB, GraphQL cost from 6 to 4
points per refresh.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Keep the main viewer-PR query open-only (cheap) but add a second
GraphQL call that aliases just the stale PRs — those linked to
active/paused/inReview sessions but no longer in the open viewer
set. Each alias is `prN: repository(...) { pullRequest(number: N) { state ... } }`,
so N stale PRs collapse into one round-trip costing ~1 GraphQL point.

This restores the merged badge in PRBadge and lets autoCompleteFinishedSessions
distinguish merged vs closed in its log. Steady state with all session PRs
open is still zero extra calls; the follow-up only fires when a session is
wrapping up. Completed sessions are excluded from the candidate set so the
follow-up doesn't re-fetch the same merged PRs every refresh.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dhilgaertner dhilgaertner merged commit f30a078 into main Apr 16, 2026
2 checks passed
@dhilgaertner dhilgaertner deleted the feature/crow-173-rate-limit-audit branch April 16, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IssueTracker: GitHub GraphQL rate-limit exceeded — audit and consolidate gh calls

2 participants