-
Notifications
You must be signed in to change notification settings - Fork 397
fix(codex): prevent infinite loop when fetching models on settings screen #455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(codex): prevent infinite loop when fetching models on settings screen #455
Conversation
…reen When Codex is not connected/authenticated, the /api/codex/models endpoint returns 503. The fetchCodexModels function had no cooldown after failures, causing infinite retries when navigating to the Settings screen. Added codexModelsLastFailedAt state to track failed fetch attempts and skip retries for 30 seconds after a failure. This prevents the infinite loop while still allowing periodic retry attempts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded per-file failure tracking and a retry cooldown for Codex models fetches in the app store. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @stefandevo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the application would enter an infinite loop of API calls when attempting to fetch Codex models if the Codex service was unavailable. By introducing a 30-second cooldown period after a failed fetch attempt, the change prevents excessive network requests and improves the application's resilience and user experience during service outages, ensuring that the UI remains responsive. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively resolves an infinite loop that occurred when fetching Codex models on the settings screen. The approach of adding a codexModelsLastFailedAt state and implementing a failure cooldown is a solid solution to prevent rapid retries on 503 errors. The code changes are clear and well-targeted. I have one suggestion to improve the maintainability of the time-based constants used for caching and cooldowns.
apps/ui/src/store/app-store.ts
Outdated
| const FAILURE_COOLDOWN = 30000; // 30 seconds | ||
| if ( | ||
| !forceRefresh && | ||
| codexModelsLastFailedAt && | ||
| Date.now() - codexModelsLastFailedAt < FAILURE_COOLDOWN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better readability and maintainability, it's good practice to express time-based constants as calculations (e.g., 30 * 1000) to make the units explicit. Appending _MS to the constant name would also improve clarity.
Additionally, for consistency, consider applying the same treatment to the magic number 300000 used for the success cache duration on line 3134. Ideally, both constants would be defined at a higher scope (e.g., outside this function) to make them easier to find and manage.
| const FAILURE_COOLDOWN = 30000; // 30 seconds | |
| if ( | |
| !forceRefresh && | |
| codexModelsLastFailedAt && | |
| Date.now() - codexModelsLastFailedAt < FAILURE_COOLDOWN | |
| const FAILURE_COOLDOWN_MS = 30 * 1000; // 30 seconds | |
| if ( | |
| !forceRefresh && | |
| codexModelsLastFailedAt && | |
| Date.now() - codexModelsLastFailedAt < FAILURE_COOLDOWN_MS |
- Rename FAILURE_COOLDOWN to FAILURE_COOLDOWN_MS with explicit calculation - Add SUCCESS_CACHE_MS constant to replace magic number 300000 - Use multiplication (30 * 1000, 5 * 60 * 1000) to make units explicit Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai can you review this PR |
🧩 Analysis chain@stefandevo, I'll review this PR for you. Let me fetch the changes first. 🏁 Script executed: Length of output: 2675 @stefandevo, I'll review this PR now! OverviewThis is a solid fix for the infinite retry loop issue. The implementation correctly addresses the root cause by adding a cooldown mechanism after failed requests. Here are my observations: ✅ Strengths
💡 Suggestions1. Consider exponential backoff for repeated failuresThe current implementation uses a fixed 30-second cooldown. If Codex remains unavailable for an extended period and users keep manually refreshing, you might want to consider exponential backoff: // Could track failure count and increase cooldown
codexModelsFailureCount: number;
// Then: FAILURE_COOLDOWN_MS * Math.pow(2, Math.min(failureCount, 5))However, for the current use case, the fixed 30-second cooldown is reasonable and simpler. 2. Add console logging for debuggingConsider adding a debug log when requests are skipped due to cooldown: if (
!forceRefresh &&
codexModelsLastFailedAt &&
Date.now() - codexModelsLastFailedAt < FAILURE_COOLDOWN_MS
) {
console.debug('Codex models fetch skipped: in failure cooldown period');
return;
}3. Document the forceRefresh bypass behaviorThe /**
* Fetches available Codex models.
* @param forceRefresh - If true, bypasses both success cache and failure cooldown
*/
fetchCodexModels: async (forceRefresh = false) => {🤔 Questions
✨ Minor NoteThe code already has protection against concurrent fetches via the VerdictLGTM! This is a well-thought-out fix that solves the immediate problem without over-engineering. The implementation is clean, maintainable, and addresses the root cause described in the PR objectives. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ui/src/store/app-store.ts (1)
3117-3173: Solid fix for the infinite retry loop.The implementation correctly prevents repeated fetch attempts by:
- Recording failure timestamps for cooldown enforcement
- Checking failure cooldown before success cache (correct ordering)
- Clearing failure state on success to enable immediate recovery
The named constants (
FAILURE_COOLDOWN_MS,SUCCESS_CACHE_MS) improve readability over magic numbers.💡 Optional: Add debug logging when fetch is skipped due to cooldown
This would help with debugging if users report delayed model loading:
// Skip if recently failed and not forcing refresh if ( !forceRefresh && codexModelsLastFailedAt && Date.now() - codexModelsLastFailedAt < FAILURE_COOLDOWN_MS ) { + logger.debug('Skipping Codex models fetch - within failure cooldown'); return; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/store/app-store.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/store/app-store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/store/app-store.ts
🔇 Additional comments (2)
apps/ui/src/store/app-store.ts (2)
692-692: LGTM!The new state field follows the existing naming and type conventions for timestamp tracking (consistent with
codexModelsLastFetched).
1290-1290: LGTM!Correct initialization to
null- consistent with other timestamp fields and appropriate for a fresh application state.
Summary
Fixes an infinite loop that occurs when navigating to the Settings screen while Codex is not connected/authenticated.
Problem
When the user is not connected to Codex, the
/api/codex/modelsendpoint returns 503 Service Unavailable. The frontend'sfetchCodexModels()function had no cooldown after failures, causing infinite retries:Root Cause
hasApiKey: truemeans "Codex is ready" - but this only means an API key is installed locally, not that the user is authenticated to the Codex servercodexModelsLastFetchedis only set on success, so there's no cooldown between failed attemptsSolution
Added a
codexModelsLastFailedAtstate field to track when the last fetch failure occurred. ThefetchCodexModels()function now:codexModelsLastFailedAtwhen a fetch failscodexModelsLastFailedAton successful fetchesThis prevents the infinite loop while still allowing periodic retry attempts.
Changes
apps/ui/src/store/app-store.ts: AddedcodexModelsLastFailedAtstate and cooldown logicTest Plan
/api/codex/modelsappear (not infinite)🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.