feat(rate-limit): update quota banner from response headers and persi…#80
Open
Ri1tik wants to merge 1 commit into
Open
feat(rate-limit): update quota banner from response headers and persi…#80Ri1tik wants to merge 1 commit into
Ri1tik wants to merge 1 commit into
Conversation
WalkthroughRate limit data propagation is refactored to an event-driven model: ChangesEvent-Driven Rate Limit Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
github-actions
Bot
added
no-issue-linked
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/RateLimitBanner.jsx`:
- Around line 36-39: The span element in RateLimitBanner directly accesses
rateLimit.used and rateLimit.reset without checking if these properties exist,
which could display "undefined" or create invalid dates if the event payload is
incomplete. Add fallback values for both rateLimit.used (such as a default value
like 0) and rateLimit.reset (such as a default value like 0) using optional
chaining operators or logical OR operators to ensure valid display content even
when these properties are undefined.
- Around line 36-39: The toLocaleTimeString() method call on the rateLimit.reset
date in the RateLimitBanner component displays only the time without timezone
information, which can confuse users about when the rate limit actually resets.
Modify the toLocaleTimeString() call to include timezone information by passing
an options object as a second parameter that specifies timeZoneName property, so
users see both the local time and their timezone abbreviation (e.g., "4:07:57 PM
PST") for better clarity across different user timezones.
In `@src/context/AppContext.jsx`:
- Around line 38-49: The handler function in the useEffect for the
'rate-limit-update' event listener directly uses e.detail without validation
before calling setRateLimit and storing it in localStorage with the key
'oe_rate_limit'. Add validation logic to verify that e.detail is a valid object
with the expected structure and properties (such as checking that it is an
object type and has required fields) before proceeding with setRateLimit and
localStorage.setItem. Only allow the state and localStorage updates to occur if
the validation passes.
In `@src/services/github.js`:
- Around line 65-74: Add null checks before converting rate limit header values
to numbers in the custom event dispatch. For each header retrieval
(x-ratelimit-limit, x-ratelimit-remaining, x-ratelimit-used, x-ratelimit-reset),
verify that the header exists before passing it to Number(). Either check if the
header value is null before conversion or provide sensible default values when
headers are missing. This prevents Number(null) from returning 0 and ensures
rate limit information displays correctly even when headers are absent.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90bb8d9d-3bbf-4e42-8d76-54077ac92a83
📒 Files selected for processing (3)
src/components/RateLimitBanner.jsxsrc/context/AppContext.jsxsrc/services/github.js
Comment on lines
+36
to
+39
| <span style={{ fontSize: 11, color: 'var(--text2)' }}> | ||
| Used: {rateLimit.used} • Reset at{' '} | ||
| {new Date(rateLimit.reset * 1000).toLocaleTimeString()} | ||
| </span> |
There was a problem hiding this comment.
Add fallback for potentially undefined rate limit properties.
If the rate-limit-update event payload is incomplete (due to the missing validation in AppContext), rateLimit.used or rateLimit.reset could be undefined, displaying "Used: undefined" or an invalid date.
🛡️ Proposed fix with fallbacks
<span style={{ fontSize: 11, color: 'var(--text2)' }}>
- Used: {rateLimit.used} • Reset at{' '}
- {new Date(rateLimit.reset * 1000).toLocaleTimeString()}
+ Used: {rateLimit.used ?? 0} • Reset at{' '}
+ {rateLimit.reset ? new Date(rateLimit.reset * 1000).toLocaleTimeString() : 'Unknown'}
</span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/RateLimitBanner.jsx` around lines 36 - 39, The span element in
RateLimitBanner directly accesses rateLimit.used and rateLimit.reset without
checking if these properties exist, which could display "undefined" or create
invalid dates if the event payload is incomplete. Add fallback values for both
rateLimit.used (such as a default value like 0) and rateLimit.reset (such as a
default value like 0) using optional chaining operators or logical OR operators
to ensure valid display content even when these properties are undefined.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding timezone and locale control for better internationalization.
The toLocaleTimeString() call uses default browser locale and no timezone information is displayed. Users in different timezones may be confused about when the rate limit actually resets, especially since GitHub's rate limits reset in UTC.
🌍 Optional enhancement for i18n
<span style={{ fontSize: 11, color: 'var(--text2)' }}>
Used: {rateLimit.used} • Reset at{' '}
- {new Date(rateLimit.reset * 1000).toLocaleTimeString()}
+ {new Date(rateLimit.reset * 1000).toLocaleTimeString(undefined, {
+ hour: '2-digit',
+ minute: '2-digit',
+ second: '2-digit',
+ timeZoneName: 'short'
+ })}
</span>This adds timezone information (e.g., "4:07:57 PM PST") to help users understand when the reset occurs in their local time.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <span style={{ fontSize: 11, color: 'var(--text2)' }}> | |
| Used: {rateLimit.used} • Reset at{' '} | |
| {new Date(rateLimit.reset * 1000).toLocaleTimeString()} | |
| </span> | |
| <span style={{ fontSize: 11, color: 'var(--text2)' }}> | |
| Used: {rateLimit.used} • Reset at{' '} | |
| {new Date(rateLimit.reset * 1000).toLocaleTimeString(undefined, { | |
| hour: '2-digit', | |
| minute: '2-digit', | |
| second: '2-digit', | |
| timeZoneName: 'short' | |
| })} | |
| </span> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/RateLimitBanner.jsx` around lines 36 - 39, The
toLocaleTimeString() method call on the rateLimit.reset date in the
RateLimitBanner component displays only the time without timezone information,
which can confuse users about when the rate limit actually resets. Modify the
toLocaleTimeString() call to include timezone information by passing an options
object as a second parameter that specifies timeZoneName property, so users see
both the local time and their timezone abbreviation (e.g., "4:07:57 PM PST") for
better clarity across different user timezones.
Source: Coding guidelines
Comment on lines
+38
to
+49
| useEffect(() => { | ||
| const handler = e => { | ||
| setRateLimit(e.detail) | ||
| localStorage.setItem('oe_rate_limit', JSON.stringify(e.detail)) | ||
| } | ||
|
|
||
| window.addEventListener('rate-limit-update', handler) | ||
|
|
||
| return () => { | ||
| window.removeEventListener('rate-limit-update', handler) | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Validate event payload to prevent malformed rate limit data.
The event handler (lines 39-42) directly uses e.detail without validation. If a malicious script or browser extension dispatches a rate-limit-update event with invalid data, it could corrupt the rate limit state and localStorage.
🔒 Proposed fix with validation
useEffect(() => {
const handler = e => {
+ const { limit, remaining, used, reset } = e.detail || {}
+
+ if (
+ typeof limit !== 'number' || limit < 0 ||
+ typeof remaining !== 'number' || remaining < 0 ||
+ typeof used !== 'number' || used < 0 ||
+ typeof reset !== 'number' || reset <= 0
+ ) {
+ console.warn('Invalid rate-limit-update event payload:', e.detail)
+ return
+ }
+
setRateLimit(e.detail)
localStorage.setItem('oe_rate_limit', JSON.stringify(e.detail))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| const handler = e => { | |
| setRateLimit(e.detail) | |
| localStorage.setItem('oe_rate_limit', JSON.stringify(e.detail)) | |
| } | |
| window.addEventListener('rate-limit-update', handler) | |
| return () => { | |
| window.removeEventListener('rate-limit-update', handler) | |
| } | |
| }, []) | |
| useEffect(() => { | |
| const handler = e => { | |
| const { limit, remaining, used, reset } = e.detail || {} | |
| if ( | |
| typeof limit !== 'number' || limit < 0 || | |
| typeof remaining !== 'number' || remaining < 0 || | |
| typeof used !== 'number' || used < 0 || | |
| typeof reset !== 'number' || reset <= 0 | |
| ) { | |
| console.warn('Invalid rate-limit-update event payload:', e.detail) | |
| return | |
| } | |
| setRateLimit(e.detail) | |
| localStorage.setItem('oe_rate_limit', JSON.stringify(e.detail)) | |
| } | |
| window.addEventListener('rate-limit-update', handler) | |
| return () => { | |
| window.removeEventListener('rate-limit-update', handler) | |
| } | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/AppContext.jsx` around lines 38 - 49, The handler function in the
useEffect for the 'rate-limit-update' event listener directly uses e.detail
without validation before calling setRateLimit and storing it in localStorage
with the key 'oe_rate_limit'. Add validation logic to verify that e.detail is a
valid object with the expected structure and properties (such as checking that
it is an object type and has required fields) before proceeding with
setRateLimit and localStorage.setItem. Only allow the state and localStorage
updates to occur if the validation passes.
Comment on lines
+65
to
+74
| window.dispatchEvent( | ||
| new CustomEvent('rate-limit-update', { | ||
| detail: { | ||
| limit: Number(res.headers.get('x-ratelimit-limit')), | ||
| remaining: Number(res.headers.get('x-ratelimit-remaining')), | ||
| used: Number(res.headers.get('x-ratelimit-used')), | ||
| reset: Number(res.headers.get('x-ratelimit-reset')) | ||
| } | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Add defensive null checks for rate limit headers.
The code converts header values using Number() without checking if the headers exist. If GitHub doesn't send rate limit headers (edge case), Number(null) returns 0, which would incorrectly display "0/0" remaining.
🛡️ Proposed fix with validation
- window.dispatchEvent(
- new CustomEvent('rate-limit-update', {
- detail: {
- limit: Number(res.headers.get('x-ratelimit-limit')),
- remaining: Number(res.headers.get('x-ratelimit-remaining')),
- used: Number(res.headers.get('x-ratelimit-used')),
- reset: Number(res.headers.get('x-ratelimit-reset'))
- }
- })
- )
+ const limit = res.headers.get('x-ratelimit-limit')
+ const remaining = res.headers.get('x-ratelimit-remaining')
+ const used = res.headers.get('x-ratelimit-used')
+ const reset = res.headers.get('x-ratelimit-reset')
+
+ if (limit && remaining && used && reset) {
+ window.dispatchEvent(
+ new CustomEvent('rate-limit-update', {
+ detail: {
+ limit: Number(limit),
+ remaining: Number(remaining),
+ used: Number(used),
+ reset: Number(reset)
+ }
+ })
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/github.js` around lines 65 - 74, Add null checks before
converting rate limit header values to numbers in the custom event dispatch. For
each header retrieval (x-ratelimit-limit, x-ratelimit-remaining,
x-ratelimit-used, x-ratelimit-reset), verify that the header exists before
passing it to Number(). Either check if the header value is null before
conversion or provide sensible default values when headers are missing. This
prevents Number(null) from returning 0 and ensures rate limit information
displays correctly even when headers are absent.
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.
Addressed Issues:
Fixes #(issue number)
Screenshots/Recordings:
Before
https://drive.google.com/file/d/1q-_7wWq25cndkLD_qype1LJuOBM2Rr1i/view?usp=drive_link
After
https://drive.google.com/file/d/13mKHtOhIFFp896vYoL8PKb-rhzp9FVnT/view?usp=drive_link
Additional Notes:
This PR improves GitHub API rate limit monitoring by:
/rate_limitAPI calls, reducing unnecessary API consumption.Benefits
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit