refactor: use /rate_limit for token validation#159
Open
bluwy wants to merge 3 commits into
Open
Conversation
|
@bluwy is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughChanged GitHub token validation to call the Changes
Sequence Diagram(s)sequenceDiagram
participant GHToken as GHToken.validate()
participant GhFetch as ghFetch / utils/github
participant GitHub as GitHub API (/rate_limit)
GHToken->>GhFetch: GET /api.github.com/rate_limit
GhFetch->>GitHub: HTTP GET /rate_limit
GitHub-->>GhFetch: 200 + JSON rate_limit
GhFetch-->>GHToken: Response
GHToken->>GHToken: updateStatusFromRateLimitEndpoint() parses JSON and sets token.reset / remaining
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
utils/github_token.ts (1)
55-60: Use the repo'sserverFetch()wrapper for this new HTTP call.This new external request bypasses the standard Nitro fetch abstraction.
As per coding guidelines,
Use serverFetch(url, opts) from nitro/app for internal and external HTTP requests (replaces $fetch).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/github_token.ts` around lines 55 - 60, The rate-limit check in utils/github_token.ts currently calls the global fetch directly (the call using fetch("https://api.github.com/rate_limit", { headers: { Authorization: `token ${this.token}` } })) which bypasses the project's Nitro fetch wrapper; replace that direct fetch invocation with the repo's serverFetch(url, opts) from nitro/app, passing the same URL and headers (including Authorization: `token ${this.token}` and User-Agent) and await its result so the call uses the standard serverFetch abstraction used across the project.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/GH_API.md:
- Around line 63-64: Update the two bullets to match current behavior: state
that ensureAllTokensValidated() bootstraps App tokens (JWT -> list installations
-> create installation tokens) before validating ghTokens, and note that ghFetch
now updates token/status inline after fetch() returns rather than relying on an
onResponse hook; reference ensureAllTokensValidated(), ghTokens, ghFetch, and
onResponse in the description so maintainers can find the related logic and
avoid reverting to the old execution order.
In `@test/github_token.test.ts`:
- Around line 22-39: Tests still use header-only mocks (mockResponse(200,
rateLimitHeaders(...))) but updateStatusFromRateLimitEndpoint() now calls
res.json() for 200 responses, causing parse errors; replace the success-path
header-only mocks with body-based mocks by using
mockRateLimitResponse(remaining, limit, resetEpoch?) instead of
mockResponse(...) in the suites that target ensureAllTokensValidated,
revalidateGHTokens, and acquireGHToken so the response includes the JSON body
the code expects.
In `@utils/github_token.ts`:
- Around line 61-67: The code currently sets this.valid = res.ok which treats
any non-2xx /rate_limit response (e.g., 403/429 secondary rate limits) as an
invalid token; update the logic in updateStatusFromResponse (or the method
handling the /rate_limit response) to only mark the token invalid when
res.status === 401, and otherwise preserve token validity while still reading
and applying rate info (parse res.json() when present and set this.remaining,
this.limit, this.reset from data.resources.core regardless of res.ok). Ensure
you do not swallow the reset bookkeeping on 403/429 and keep the existing
handling for 401 to mark this.valid = false.
---
Nitpick comments:
In `@utils/github_token.ts`:
- Around line 55-60: The rate-limit check in utils/github_token.ts currently
calls the global fetch directly (the call using
fetch("https://api.github.com/rate_limit", { headers: { Authorization: `token
${this.token}` } })) which bypasses the project's Nitro fetch wrapper; replace
that direct fetch invocation with the repo's serverFetch(url, opts) from
nitro/app, passing the same URL and headers (including Authorization: `token
${this.token}` and User-Agent) and await its result so the call uses the
standard serverFetch abstraction used across the project.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a296491b-2055-46ef-a549-0df7d1792761
📒 Files selected for processing (4)
.agents/GH_API.mdtest/github_token.test.tsutils/github.tsutils/github_token.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
utils/github_token.ts (1)
54-67:⚠️ Potential issue | 🟠 MajorStill skip header-based quota updates on
/rate_limitfailures.
utils/github.ts:38-56updates token state from every GitHub response before checkingres.ok, but this helper only reads/rate_limitJSON on success. GitHub documents that rate-limit breaches can return403/429, and that callers should use the rate-limit headers when possible to decide when to retry. BecauseupdateStatusFromResponse(res)is never called here, a spent token can keep staleremaining/resetstate and still lookavailable. (docs.github.com)🩹 Suggested fix
async updateStatusFromRateLimitEndpoint() { const res = await fetch("https://api.github.com/rate_limit", { headers: { "User-Agent": "fetch", Authorization: `token ${this.token}`, }, }); - this.valid = res.status !== 401; + this.updateStatusFromResponse(res); if (res.ok) { const data = await res.json(); this.remaining = data.resources.core.remaining; this.limit = data.resources.core.limit; this.reset = data.resources.core.reset * 1000; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/github_token.ts` around lines 54 - 67, The updateStatusFromRateLimitEndpoint method currently only parses JSON when res.ok and therefore never calls updateStatusFromResponse(res) to update header-derived quota info; modify updateStatusFromRateLimitEndpoint to always call this.updateStatusFromResponse(res) immediately after receiving the Response (before the res.ok check) so header-based remaining/limit/reset are applied even on 403/429 responses, and then only parse JSON to overwrite values when res.ok and JSON is present; reference updateStatusFromRateLimitEndpoint, updateStatusFromResponse, this.token, and this.valid to locate and update the logic.
🧹 Nitpick comments (1)
utils/github_token.ts (1)
55-60: UseserverFetch()for the new GitHub probe.This PR adds another outbound HTTP path with raw
fetch. Please route it throughserverFetch()so token validation follows the repo's Nitro HTTP abstraction.As per coding guidelines, "Use
serverFetch(url, opts)fromnitro/appfor internal and external HTTP requests (replaces$fetch)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/github_token.ts` around lines 55 - 60, Replace the raw fetch call in utils/github_token.ts with the repo's Nitro HTTP abstraction by using serverFetch instead: import serverFetch from 'nitro/app' (or the project's canonical export) and call serverFetch("https://api.github.com/rate_limit", { headers: { "User-Agent": "fetch", Authorization: `token ${this.token}` } }) in place of fetch so the token validation path in the method that uses this.token (token validation / rate limit probe) goes through the centralized HTTP layer; preserve the existing await, response handling, and error paths as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@utils/github_token.ts`:
- Around line 54-67: The updateStatusFromRateLimitEndpoint method currently only
parses JSON when res.ok and therefore never calls updateStatusFromResponse(res)
to update header-derived quota info; modify updateStatusFromRateLimitEndpoint to
always call this.updateStatusFromResponse(res) immediately after receiving the
Response (before the res.ok check) so header-based remaining/limit/reset are
applied even on 403/429 responses, and then only parse JSON to overwrite values
when res.ok and JSON is present; reference updateStatusFromRateLimitEndpoint,
updateStatusFromResponse, this.token, and this.valid to locate and update the
logic.
---
Nitpick comments:
In `@utils/github_token.ts`:
- Around line 55-60: Replace the raw fetch call in utils/github_token.ts with
the repo's Nitro HTTP abstraction by using serverFetch instead: import
serverFetch from 'nitro/app' (or the project's canonical export) and call
serverFetch("https://api.github.com/rate_limit", { headers: { "User-Agent":
"fetch", Authorization: `token ${this.token}` } }) in place of fetch so the
token validation path in the method that uses this.token (token validation /
rate limit probe) goes through the centralized HTTP layer; preserve the existing
await, response handling, and error paths as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3925d1d7-3bb3-48e3-bc52-c2de1a43202c
📒 Files selected for processing (2)
test/github_token.test.tsutils/github_token.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/github_token.test.ts
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.
This PR uses
/rate_limitendpoint for token validation as the endpoint doesn't count against the limit compared to/meta.This means we're free to refresh the status page without any impact, though perhaps we still want to put it behind auth to prevent abuse. It also means we can easily show the rate limit for other github apis, like graphql, in the future.
With this PR, there's
updateStatusFromResponseandupdateStatusFromRateLimitEndpointto update the token status.Summary by CodeRabbit
Documentation
Refactor
Tests