fix: wire up API quota refresh button#79
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesAPI Quota Refresh Feature
Estimated code review effort: 2 (Simple) | ~8 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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
frontend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/pages/SettingsPage.jsx`:
- Around line 208-218: The refresh icon (FiRefreshCw) is implemented as a
clickable element that lacks proper keyboard accessibility, preventing
keyboard-only users from accessing the refresh functionality. Replace the icon
with a semantic HTML button element that wraps the FiRefreshCw icon, moving the
onClick handler with the isRefreshing check, setIsRefreshing state management,
and refreshRateLimit call to the button element. This ensures the control is
properly focusable via keyboard and operable with Enter/Space keys while
maintaining the visual styling and rotation animation behavior.
🪄 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: 80562388-23b1-40f5-85b8-e7c4e8fe5cd5
📒 Files selected for processing (2)
src/context/AppContext.jsxsrc/pages/SettingsPage.jsx
|
@coderabbitai Great catch on accessibility! I have updated the code to wrap the |
github-actions
Bot
added
size/S
|
Tip For best results, initiate chat on the files or code changes.
(◕‿◕✿) 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
github-actions
Bot
added
size/S
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/SettingsPage.jsx (1)
210-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure
isRefreshingis always reset on refresh failure.If
refreshRateLimit()throws (Line 213), the reset path at Line 214 is skipped, leavingisRefreshingstucktrueand effectively disabling future refresh attempts.Proposed fix
onClick={async () => { - if (isRefreshing) return; - setIsRefreshing(true); - await refreshRateLimit(); - setTimeout(() => setIsRefreshing(false), 500); // Minimum spin duration for visual feedback + if (isRefreshing) return + setIsRefreshing(true) + try { + await refreshRateLimit() + } finally { + setTimeout(() => setIsRefreshing(false), 500) // Minimum spin duration for visual feedback + } }}🤖 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/pages/SettingsPage.jsx` around lines 210 - 215, The onClick handler for the refresh button does not handle failures in the refreshRateLimit() call, which means if refreshRateLimit() throws an error, the setTimeout that resets isRefreshing to false will never execute, leaving the state stuck at true and preventing future refresh attempts. Wrap the await refreshRateLimit() call in a try-catch block (or use a finally block) to ensure that setIsRefreshing(false) is always called to reset the loading state, regardless of whether the refresh succeeds or fails.
🤖 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.
Duplicate comments:
In `@src/pages/SettingsPage.jsx`:
- Around line 210-215: The onClick handler for the refresh button does not
handle failures in the refreshRateLimit() call, which means if
refreshRateLimit() throws an error, the setTimeout that resets isRefreshing to
false will never execute, leaving the state stuck at true and preventing future
refresh attempts. Wrap the await refreshRateLimit() call in a try-catch block
(or use a finally block) to ensure that setIsRefreshing(false) is always called
to reset the loading state, regardless of whether the refresh succeeds or fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7f99aec-66e9-4d22-8465-96b7bacfbc07
📒 Files selected for processing (1)
src/pages/SettingsPage.jsx
|
@coderabbitai Nice catch on the missing error handling! I've wrapped the |
github-actions
Bot
added
size/S
|
Tip For best results, initiate chat on the files or code changes.
(✿◠‿◠) 🐇 |
|
Hi @Zahnentferner, I've finished implementing this fix and addressing the automated review feedback. All CI checks are passing green and CodeRabbit has approved the changes. Could you please take a look when you have a free moment? Let me know if you'd like any further adjustments! Thanks. |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
github-actions
Bot
added
size/S
|
@Dotify71 Please resolve the merge conflicts, so that I can review and merge your PR. |
github-actions
Bot
added
size/S
|
@rahul-vyas-dev Resolved the conflicts |
|
@Dotify71 share any visual, If you can. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/context/AppContext.jsx`:
- Around line 63-66: `refreshRateLimit` in `AppContext` currently swallows
`fetchRateLimit` failures because `fetchRateLimit(pat)` returns `null` and the
callback just no-ops. Update `fetchRateLimit` and `refreshRateLimit` to
propagate failure explicitly, either by returning a success boolean/nullable
result that callers can inspect or by rejecting on error, and then adjust
`SettingsPage` to consume that result and show a brief error indicator instead
of silently finishing the refresh.
In `@src/pages/SettingsPage.jsx`:
- Around line 211-241: The refresh quota button in SettingsPage needs two
follow-ups: add a disabled state tied to isRefreshing on the button so it is not
focusable/announced as interactive during an in-flight refresh, and replace the
hardcoded hover:bg-zinc-800 styling with the same CSS-variable-based hover
pattern used by the sibling icon button in this file. Update the button block
around the onClick handler and FiRefreshCw usage to keep the existing
guard/try-finally behavior while aligning accessibility and theming.
🪄 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: 6ab7f6ef-5d85-4056-83b4-c103872b6e16
📒 Files selected for processing (2)
src/context/AppContext.jsxsrc/pages/SettingsPage.jsx
github-actions
Bot
added
size/M
|
@rahul-vyas-dev I tried to on this but unfortunately it shows white screen before it turned on properly but this time i don't know whats the matter i have seen results by my own this code is correct |
Check this and try to fix the bug, and even after this if you face the problem then let us know, and make sure to share any ss or screen recording to know about the bug or working code. |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
|
@Dotify71 are you available to solve the conflicts? If not, then I need to close the PR and if yes then solve and share visuals of working code. |
|
Ok i will add the visuals before the morning |
github-actions
Bot
added
size/M
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/context/AppContext.jsx`:
- Around line 62-69: The manual quota refresh path in refreshRateLimit updates
state but does not persist the new value, unlike the rate-limit-update event
handler. Update refreshRateLimit in AppContext to also write the fetched rl into
localStorage with the same oe_rate_limit key whenever fetchRateLimit(pat)
succeeds, keeping the cached value in sync with setRateLimit and
getStoredRateLimit.
- Around line 187-192: The Ctx.Provider value in AppContext is created inline,
so every AppProvider render produces a new object reference and re-renders all
useApp() consumers. Memoize the provider value in AppContext (for example around
the value passed to Ctx.Provider) and include the referenced state/actions such
as pat, savePat, orgs, model, issuesData, rateLimit, loading, loadMsg,
govLoading, error, totalRepo, explore, runAudit, setError, refreshRateLimit, and
staleRepoStats so the object identity stays stable unless one of those inputs
changes.
🪄 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: 832ee8f5-9298-4b20-a8e8-ad13c0c797c1
📒 Files selected for processing (2)
src/context/AppContext.jsxsrc/pages/SettingsPage.jsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/context/AppContext.jsx`:
- Around line 62-69: The manual quota refresh path in refreshRateLimit updates
state but does not persist the new value, unlike the rate-limit-update event
handler. Update refreshRateLimit in AppContext to also write the fetched rl into
localStorage with the same oe_rate_limit key whenever fetchRateLimit(pat)
succeeds, keeping the cached value in sync with setRateLimit and
getStoredRateLimit.
- Around line 187-192: The Ctx.Provider value in AppContext is created inline,
so every AppProvider render produces a new object reference and re-renders all
useApp() consumers. Memoize the provider value in AppContext (for example around
the value passed to Ctx.Provider) and include the referenced state/actions such
as pat, savePat, orgs, model, issuesData, rateLimit, loading, loadMsg,
govLoading, error, totalRepo, explore, runAudit, setError, refreshRateLimit, and
staleRepoStats so the object identity stays stable unless one of those inputs
changes.
🪄 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: 832ee8f5-9298-4b20-a8e8-ad13c0c797c1
📒 Files selected for processing (2)
src/context/AppContext.jsxsrc/pages/SettingsPage.jsx
🛑 Comments failed to post (2)
src/context/AppContext.jsx (2)
62-69: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Manual refresh doesn't persist to
localStorage, unlike the event-driven update path.The previous silent-failure issue is now correctly resolved via the boolean return. However,
refreshRateLimitonly callssetRateLimit(rl)on success, while therate-limit-updateevent handler (lines 40-42) additionally persists the value withlocalStorage.setItem('oe_rate_limit', ...). If a user manually refreshes quota and then reloads the page shortly after,getStoredRateLimit()will read a stale/old cached value instead of the freshly fetched one, since the manual refresh never updatedoe_rate_limit.♻️ Suggested fix
const refreshRateLimit = useCallback(async () => { const rl = await fetchRateLimit(pat) if (rl) { setRateLimit(rl) + localStorage.setItem('oe_rate_limit', JSON.stringify(rl)) return true } return false }, [pat])📝 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.const refreshRateLimit = useCallback(async () => { const rl = await fetchRateLimit(pat) if (rl) { setRateLimit(rl) localStorage.setItem('oe_rate_limit', JSON.stringify(rl)) return true } return false }, [pat])🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 64-64: Avoid using the initial state variable in setState
Context: setRateLimit(rl)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.(setstate-same-var)
🤖 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 62 - 69, The manual quota refresh path in refreshRateLimit updates state but does not persist the new value, unlike the rate-limit-update event handler. Update refreshRateLimit in AppContext to also write the fetched rl into localStorage with the same oe_rate_limit key whenever fetchRateLimit(pat) succeeds, keeping the cached value in sync with setRateLimit and getStoredRateLimit.
187-192: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Context value is rebuilt on every render, forcing all consumers to re-render.
Flagged by static analysis (React Doctor): the
Ctx.Providervalueobject is constructed inline, so everyAppProviderre-render (e.g. fromissuesData,loading, orgovLoadingchanges) produces a brand-new object reference and re-renders everyuseApp()consumer, regardless of which slice of state they actually use.♻️ Suggested fix
+ const contextValue = useMemo(() => ({ + pat, savePat, orgs, model, issuesData, + rateLimit, loading, loadMsg, govLoading, error, totalRepo, + explore, runAudit, setError, refreshRateLimit, staleRepoStats, + }), [pat, savePat, orgs, model, issuesData, rateLimit, loading, loadMsg, govLoading, error, totalRepo, explore, runAudit, refreshRateLimit, staleRepoStats]) + return ( - <Ctx.Provider value={{ - pat, savePat, orgs, model, issuesData, - rateLimit, loading, loadMsg, govLoading, error, totalRepo, - explore, runAudit, setError, refreshRateLimit, staleRepoStats, - }}> + <Ctx.Provider value={contextValue}> {children} </Ctx.Provider> )As per coding guidelines, "The code adheres to best practices associated with React."
📝 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.const contextValue = useMemo(() => ({ pat, savePat, orgs, model, issuesData, rateLimit, loading, loadMsg, govLoading, error, totalRepo, explore, runAudit, setError, refreshRateLimit, staleRepoStats, }), [pat, savePat, orgs, model, issuesData, rateLimit, loading, loadMsg, govLoading, error, totalRepo, explore, runAudit, setError, refreshRateLimit, staleRepoStats]) return ( <Ctx.Provider value={contextValue}> {children} </Ctx.Provider> )🧰 Tools
🪛 React Doctor (0.5.8)
[warning] 188-188: Every reader of this context redraws on each render because you build its
valueinline.Wrap the context value in
useMemoor move it outside the component so consumers do not redraw every render.(jsx-no-constructed-context-values)
🤖 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 187 - 192, The Ctx.Provider value in AppContext is created inline, so every AppProvider render produces a new object reference and re-renders all useApp() consumers. Memoize the provider value in AppContext (for example around the value passed to Ctx.Provider) and include the referenced state/actions such as pat, savePat, orgs, model, issuesData, rateLimit, loading, loadMsg, govLoading, error, totalRepo, explore, runAudit, setError, refreshRateLimit, and staleRepoStats so the object identity stays stable unless one of those inputs changes.Sources: Coding guidelines, Linters/SAST tools
github-actions
Bot
added
size/M
Screen.Recording.2026-07-03.at.10.18.27.PM.movhey @rahul-vyas-dev here is the visuals if anything else is needed just tell me i will do it. |
2 participants
Fixes #78
Description
Wired up the "dead" API Quota refresh button on the Settings page so that users can actually click it to fetch their updated GitHub rate limits without needing to trigger a full page reload or a new organization query.
Solution
refreshRateLimitcallback fromAppContext.jsx.onClickhandler to theFiRefreshCwicon inSettingsPage.jsx.transform: rotateanimation to provide visual feedback while the API call is in flight.Summary by CodeRabbit
Release Notes