Feature/CLAUDE rate limit countdown#694
Open
ProaFilippi wants to merge 3 commits intoRunMaestro:rcfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds rate-limit detection and caching in the Claude output parser, a timezone-aware reset-time extractor, AgentError.rateLimitResetAt plumbing across types and handlers, UI countdown/auto-retry in the AgentError modal, and a retry workflow that reconstructs and re-dispatches the original user message. Changes
Sequence DiagramsequenceDiagram
participant Parser as Parser (claude-output-parser)
participant Cache as Parser Cache
participant Stdout as StdoutHandler
participant Renderer as AgentErrorModal (UI)
participant Store as AgentStore
Parser->>Parser: Receive `rate_limit_event` payload
Parser->>Cache: Store resetsAt (ms) in lastRateLimitResetAt
Parser->>Stdout: Emit normalized `system` event
Parser->>Parser: Detect `rate_limited` error (assistant output)
Parser->>Cache: Read lastRateLimitResetAt (if future)
Parser->>Stdout: Emit `agent-error` with rateLimitResetAt
Stdout->>Renderer: onAgentError / listener updates state
Renderer->>Renderer: Compute effective resetAt (explicit or fallback)
Renderer->>Renderer: Render RateLimitCountdown
Renderer->>Store: onComplete -> trigger retry action
Store->>Store: Reconstruct last user message from logs
Store->>Parser: Re-dispatch queued item for processing
Parser->>Parser: Process retried message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 8
🧹 Nitpick comments (1)
src/main/preload/process.ts (1)
114-128: Stop re-declaringAgentErrorhere.This copy already drifts from
src/shared/types.ts:typeis widened tostringhere,parsedJsonis missing, and every new field now has to be patched in multiple places. Re-exporting the shared type at the preload boundary would keep main, preload, and renderer locked to the same IPC contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/process.ts` around lines 114 - 128, Remove the local AgentError interface declaration and re-export the single shared definition instead: replace the inline AgentError type in this file with an import/export from the centralized shared types module (use the existing shared AgentError symbol) so preload, main, and renderer share the exact same contract; update any local references to rely on the imported AgentError and delete the duplicated declaration to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/parsers/rate-limit-event.test.ts`:
- Around line 83-85: The test's current assertion for
agentError!.rateLimitResetAt only checks it isn't older than 24h; change it to
assert the value is defined, strictly in the future, and within the next 24
hours by replacing the single toBeGreaterThan(Date.now() - 86400000) check with
two assertions: one that agentError!.rateLimitResetAt is greater than Date.now()
and another that it is less than Date.now() + 86400000 so past reset times will
fail; keep the existing expect(agentError!.rateLimitResetAt).toBeDefined() and
target the agentError!.rateLimitResetAt symbol in the test.
In `@src/main/parsers/claude-output-parser.ts`:
- Around line 393-399: The mixed-line fallback currently reparses errorText for
rate limit resets and ignores any previously cached timestamp; update the logic
in the branch handling match.type === 'rate_limited' so that if
lastRateLimitResetAt (the cached rate_limit_event timestamp) is present it is
assigned to mixedError.rateLimitResetAt instead of re-parsing with
parseRateLimitResetTime(errorText), otherwise fall back to
parseRateLimitResetTime; adjust references to match.type, lastRateLimitResetAt,
parseRateLimitResetTime, and mixedError.rateLimitResetAt accordingly so the
cached timestamp is preserved for mixed stderr lines.
In `@src/main/parsers/error-patterns.ts`:
- Around line 1093-1118: The bug is using the current UTC offset (tzOffsetMs
computed from now/utcNow/wallNowMs) and treating now+86400000 as "tomorrow" in
the target zone, which breaks across DST; instead compute the timezone offset
and date parts for the actual target wall-clock instant. Concretely: when you
set targetDay/targetMonth/targetYear (using formatter.formatToParts on
tomorrow), compute the tzOffsetMs from that same target wall-clock instant
(i.e., format the target Date with formatter to get wallTargetParts and derive
wallTargetMs and the corresponding UTC ms), then use that target-specific offset
to convert wallTargetMs to targetUtcMs (replace using utcNow/wallNowMs with
computations based on the target instant). Update the calculations that produce
tzOffsetMs, wallTargetMs and targetUtcMs to use the target instant's offset
rather than now's.
- Line 1027: The RATE_LIMIT_RESET_REGEX currently pre-filters time zone names
with ([A-Za-z_/]+), which rejects valid zones like "America/Port-au-Prince" and
"Etc/GMT+1"; update RATE_LIMIT_RESET_REGEX to stop restricting characters inside
the parentheses and instead capture any sequence up to the closing parenthesis
(e.g., replace the character class with a "anything but )" capture) so the later
Intl.DateTimeFormat validation can accept and validate real zone names; locate
RATE_LIMIT_RESET_REGEX and modify its final capture group accordingly.
In `@src/renderer/components/AgentErrorModal.tsx`:
- Around line 238-244: The current logic treats an expired parsed reset time as
if none was provided and falls back to error.timestamp +
DEFAULT_RATE_LIMIT_WAIT_MS; change it so if error.rateLimitResetAt is present
but <= Date.now() you do not start a fresh fallback—return null (or the
immediate retry sentinel used by this component) to indicate no wait.
Concretely, in AgentErrorModal.tsx update the block that checks
error.rateLimitResetAt to: if it exists and is in the future return it; if it
exists and is in the past return null; only compute const fallback =
error.timestamp + DEFAULT_RATE_LIMIT_WAIT_MS when error.rateLimitResetAt is
undefined. This uses the existing symbols error.rateLimitResetAt,
error.timestamp, and DEFAULT_RATE_LIMIT_WAIT_MS to locate and fix the logic.
In `@src/renderer/stores/agentStore.ts`:
- Around line 226-242: The current retry guard requires lastUserLog.text which
blocks image-only messages from being retried; update the check and queued item
construction so image-only logs are allowed: change the guard to allow retry
when lastUserLog has either text or non-empty images (e.g., if (!lastUserLog ||
(!lastUserLog.text && !lastUserLog.images?.length) ...)), and build the
QueuedItem with text: lastUserLog.text ?? '' (or omit/undefined if your
QueuedItem type allows), leaving images intact so processQueuedItem (which
synthesizes DEFAULT_IMAGE_ONLY_PROMPT) can handle image-only retries; reference
symbols: lastUserLog, retryAfterError, QueuedItem, processQueuedItem,
DEFAULT_IMAGE_ONLY_PROMPT, generateId, targetTab.id.
- Around line 211-220: The code clears the agent error state
(get().clearAgentError(sessionId)) before validating that
session.agentErrorTabId still points to a live tab, which can drop the modal and
set session to idle with no retry queued; to fix, first resolve the target tab
by evaluating session.agentErrorTabId, locating the tab via session.aiTabs or
falling back to getActiveTab(session), and if no valid targetTab exists return
early, and only after confirming targetTab call get().clearAgentError(sessionId)
so the retry flow can be queued against a live tab (referencing clearAgentError,
agentErrorTabId, session.aiTabs, getActiveTab).
- Around line 282-287: The retry path currently swallows failures by only
console.logging inside the setTimeout callback that calls
get().processQueuedItem(sessionId, queuedItem, deps); import captureException
and captureMessage from src/renderer/utils/sentry.ts, call captureException(err,
{ sessionId, queuedItemId: queuedItem?.id }) and captureMessage with a short
context string when the promise rejects, and then restore a recoverable error
state (e.g. set a retryFailed flag or push the error into the agentStore's error
queue) or rethrow the error so it surfaces to upstream error handling; update
the callback around get().processQueuedItem(...) to report via
captureException/captureMessage and update agentStore state (or rethrow) instead
of only console.error.
---
Nitpick comments:
In `@src/main/preload/process.ts`:
- Around line 114-128: Remove the local AgentError interface declaration and
re-export the single shared definition instead: replace the inline AgentError
type in this file with an import/export from the centralized shared types module
(use the existing shared AgentError symbol) so preload, main, and renderer share
the exact same contract; update any local references to rely on the imported
AgentError and delete the duplicated declaration to avoid drift.
🪄 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: CHILL
Plan: Pro
Run ID: 8c75b05b-47af-4776-8718-7100ee509616
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
src/__tests__/main/parsers/rate-limit-event.test.tssrc/main/parsers/claude-output-parser.tssrc/main/parsers/error-patterns.tssrc/main/preload/process.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/types.tssrc/renderer/components/AgentErrorModal.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentListeners.tssrc/renderer/stores/agentStore.tssrc/shared/types.ts
Comment on lines
+393
to
+399
| // For rate-limit errors, try to parse the reset time | ||
| if (match.type === 'rate_limited') { | ||
| const resetAt = parseRateLimitResetTime(errorText); | ||
| if (resetAt) { | ||
| mixedError.rateLimitResetAt = resetAt; | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the cached rate_limit_event timestamp in the mixed-line fallback.
This branch re-parses errorText but ignores lastRateLimitResetAt. If Claude sends rate_limit_event first and the actual failure arrives as a mixed stderr line, the exact reset time is lost and the renderer falls back to the synthetic wait window.
♻️ Suggested fix
// For rate-limit errors, try to parse the reset time
if (match.type === 'rate_limited') {
- const resetAt = parseRateLimitResetTime(errorText);
+ let resetAt: number | null = null;
+ if (this.lastRateLimitResetAt && this.lastRateLimitResetAt > Date.now()) {
+ resetAt = this.lastRateLimitResetAt;
+ this.lastRateLimitResetAt = null;
+ }
+ if (!resetAt) {
+ resetAt = parseRateLimitResetTime(errorText);
+ }
if (resetAt) {
mixedError.rateLimitResetAt = resetAt;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/parsers/claude-output-parser.ts` around lines 393 - 399, The
mixed-line fallback currently reparses errorText for rate limit resets and
ignores any previously cached timestamp; update the logic in the branch handling
match.type === 'rate_limited' so that if lastRateLimitResetAt (the cached
rate_limit_event timestamp) is present it is assigned to
mixedError.rateLimitResetAt instead of re-parsing with
parseRateLimitResetTime(errorText), otherwise fall back to
parseRateLimitResetTime; adjust references to match.type, lastRateLimitResetAt,
parseRateLimitResetTime, and mixedError.rateLimitResetAt accordingly so the
cached timestamp is preserved for mixed stderr lines.
| * 3: optional am/pm designator | ||
| * 4: IANA timezone (e.g. "America/Winnipeg") | ||
| */ | ||
| const RATE_LIMIT_RESET_REGEX = /resets?\s+(\d{1,2})(?::(\d{2}))?\s*(am|pm)?\s*\(([A-Za-z_/]+)\)/i; |
There was a problem hiding this comment.
Don’t pre-filter valid time zones in the regex.
([A-Za-z_/]+) excludes legal zone names such as America/Port-au-Prince and Etc/GMT+1, so those users never get an exact reset timestamp even though the later Intl.DateTimeFormat validation can handle them.
Suggested fix
-const RATE_LIMIT_RESET_REGEX = /resets?\s+(\d{1,2})(?::(\d{2}))?\s*(am|pm)?\s*\(([A-Za-z_/]+)\)/i;
+const RATE_LIMIT_RESET_REGEX =
+ /resets?\s+(\d{1,2})(?::(\d{2}))?\s*(am|pm)?\s*\(([^)]+)\)/i;📝 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
| const RATE_LIMIT_RESET_REGEX = /resets?\s+(\d{1,2})(?::(\d{2}))?\s*(am|pm)?\s*\(([A-Za-z_/]+)\)/i; | |
| const RATE_LIMIT_RESET_REGEX = | |
| /resets?\s+(\d{1,2})(?::(\d{2}))?\s*(am|pm)?\s*\(([^)]+)\)/i; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/parsers/error-patterns.ts` at line 1027, The RATE_LIMIT_RESET_REGEX
currently pre-filters time zone names with ([A-Za-z_/]+), which rejects valid
zones like "America/Port-au-Prince" and "Etc/GMT+1"; update
RATE_LIMIT_RESET_REGEX to stop restricting characters inside the parentheses and
instead capture any sequence up to the closing parenthesis (e.g., replace the
character class with a "anything but )" capture) so the later
Intl.DateTimeFormat validation can accept and validate real zone names; locate
RATE_LIMIT_RESET_REGEX and modify its final capture group accordingly.
Comment on lines
+238
to
+244
| if (error.rateLimitResetAt && error.rateLimitResetAt > Date.now()) { | ||
| return error.rateLimitResetAt; | ||
| } | ||
|
|
||
| // Fallback: 60 seconds from when the error occurred | ||
| const fallback = error.timestamp + DEFAULT_RATE_LIMIT_WAIT_MS; | ||
| return fallback > Date.now() ? fallback : null; |
There was a problem hiding this comment.
Don’t start a fresh 2-hour fallback once the exact reset time has already passed.
When error.rateLimitResetAt exists but is now in the past, this code ignores it and schedules error.timestamp + 2h instead. That turns a rate limit that is ready to retry into a brand-new multi-hour delay. Only use the fallback when no parsed reset time was provided; if the parsed timestamp is expired, retry immediately instead of synthesizing another wait.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AgentErrorModal.tsx` around lines 238 - 244, The
current logic treats an expired parsed reset time as if none was provided and
falls back to error.timestamp + DEFAULT_RATE_LIMIT_WAIT_MS; change it so if
error.rateLimitResetAt is present but <= Date.now() you do not start a fresh
fallback—return null (or the immediate retry sentinel used by this component) to
indicate no wait. Concretely, in AgentErrorModal.tsx update the block that
checks error.rateLimitResetAt to: if it exists and is in the future return it;
if it exists and is in the past return null; only compute const fallback =
error.timestamp + DEFAULT_RATE_LIMIT_WAIT_MS when error.rateLimitResetAt is
undefined. This uses the existing symbols error.rateLimitResetAt,
error.timestamp, and DEFAULT_RATE_LIMIT_WAIT_MS to locate and fix the logic.
Comment on lines
+226
to
+242
| if (!lastUserLog || !lastUserLog.text) { | ||
| console.warn('[retryAfterError] No user message found to retry.'); | ||
| return; | ||
| } | ||
|
|
||
| // 4. Re-construct a QueuedItem 'message' to re-dispatch. | ||
| // By sending it as a 'message' (even if it originally was a command), | ||
| // we avoid double-substituting template variables, as the text in the log | ||
| // is already the final rendered prompt. Also, processQueuedItem does not | ||
| // push duplicate logs for 'message' types. | ||
| const queuedItem: QueuedItem = { | ||
| id: generateId(), | ||
| timestamp: Date.now(), | ||
| tabId: targetTab.id, | ||
| type: 'message', | ||
| text: lastUserLog.text, | ||
| images: lastUserLog.images, |
There was a problem hiding this comment.
Image-only requests can’t be retried from this path.
This guard requires lastUserLog.text, but processQueuedItem() already accepts image-only message items and synthesizes DEFAULT_IMAGE_ONLY_PROMPT at Lines 376-383. A rate-limited screenshot upload will never auto-retry here.
♻️ Suggested fix
- if (!lastUserLog || !lastUserLog.text) {
+ if (!lastUserLog || (!lastUserLog.text && !lastUserLog.images?.length)) {
console.warn('[retryAfterError] No user message found to retry.');
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/stores/agentStore.ts` around lines 226 - 242, The current retry
guard requires lastUserLog.text which blocks image-only messages from being
retried; update the check and queued item construction so image-only logs are
allowed: change the guard to allow retry when lastUserLog has either text or
non-empty images (e.g., if (!lastUserLog || (!lastUserLog.text &&
!lastUserLog.images?.length) ...)), and build the QueuedItem with text:
lastUserLog.text ?? '' (or omit/undefined if your QueuedItem type allows),
leaving images intact so processQueuedItem (which synthesizes
DEFAULT_IMAGE_ONLY_PROMPT) can handle image-only retries; reference symbols:
lastUserLog, retryAfterError, QueuedItem, processQueuedItem,
DEFAULT_IMAGE_ONLY_PROMPT, generateId, targetTab.id.
Comment on lines
+282
to
+287
| setTimeout(() => { | ||
| get() | ||
| .processQueuedItem(sessionId, queuedItem, deps) | ||
| .catch((err) => { | ||
| console.error('[retryAfterError] Failed to retry item:', err); | ||
| }); |
There was a problem hiding this comment.
Don’t swallow async retry failures with a console-only catch.
If the reconstructed retry rejects, this path only logs after the original error has already been cleared. Please capture/report it and restore a recoverable error state or surface a user-visible failure. As per coding guidelines, "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production." and "Use Sentry utilities for explicit reporting: import captureException and captureMessage from src/main/utils/sentry.ts or src/renderer/utils/sentry.ts to report exceptions with context and notable events."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/stores/agentStore.ts` around lines 282 - 287, The retry path
currently swallows failures by only console.logging inside the setTimeout
callback that calls get().processQueuedItem(sessionId, queuedItem, deps); import
captureException and captureMessage from src/renderer/utils/sentry.ts, call
captureException(err, { sessionId, queuedItemId: queuedItem?.id }) and
captureMessage with a short context string when the promise rejects, and then
restore a recoverable error state (e.g. set a retryFailed flag or push the error
into the agentStore's error queue) or rethrow the error so it surfaces to
upstream error handling; update the callback around get().processQueuedItem(...)
to report via captureException/captureMessage and update agentStore state (or
rethrow) instead of only console.error.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/renderer/stores/agentStore.ts (2)
226-229:⚠️ Potential issue | 🟡 MinorImage-only requests cannot be retried from this path.
This guard requires
lastUserLog.text, butprocessQueuedItem()handles image-only messages by synthesizingDEFAULT_IMAGE_ONLY_PROMPT(see lines 376-383). A rate-limited screenshot upload will never auto-retry.♻️ Suggested fix
- if (!lastUserLog || !lastUserLog.text) { + if (!lastUserLog || (!lastUserLog.text && !lastUserLog.images?.length)) { console.warn('[retryAfterError] No user message found to retry.'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/agentStore.ts` around lines 226 - 229, The retryAfterError guard rejects image-only requests because it requires lastUserLog.text; update retryAfterError to treat image-only queued messages the same as processQueuedItem by falling back to the synthesized prompt: if lastUserLog.text is falsy but the log represents an image-only upload (or the queue item lacks text), use DEFAULT_IMAGE_ONLY_PROMPT before returning, or check for the same image-only flag/condition used by processQueuedItem so rate-limited screenshot uploads can be retried; reference retryAfterError, processQueuedItem, lastUserLog, and DEFAULT_IMAGE_ONLY_PROMPT to implement the parity.
282-288:⚠️ Potential issue | 🟠 MajorAsync retry failures are silently swallowed with console-only logging.
Per coding guidelines, unexpected errors should be reported to Sentry rather than only logged. Additionally, after clearing the error state at line 220, a failed retry leaves the user with no error modal and no indication of failure.
♻️ Suggested fix - report to Sentry and restore error state
+import { captureException } from '../utils/sentry'; // In retryAfterError: setTimeout(() => { get() .processQueuedItem(sessionId, queuedItem, deps) .catch((err) => { - console.error('[retryAfterError] Failed to retry item:', err); + captureException(err, { + extra: { sessionId, queuedItemId: queuedItem.id, operation: 'retryAfterError' }, + }); + console.error('[retryAfterError] Failed to retry item:', err); + // Reset session to idle so user can try again manually + useSessionStore.getState().setSessions((prev) => + prev.map((s) => (s.id !== sessionId ? s : { ...s, state: 'idle' as SessionState, busySource: undefined })) + ); }); }, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/agentStore.ts` around lines 282 - 288, The retry callback currently swallows failures (only console.error); update the catch block for get().processQueuedItem(sessionId, queuedItem, deps) to report the error to Sentry (e.g., call Sentry.captureException(err)) and then restore the session's error state so the user sees the error modal again by reusing the store's existing error setter (the same method/property used earlier to clear the error state), e.g., call get().setSessionError(sessionId, err) or the appropriate restoreError method after capturing the exception; keep references to processQueuedItem and get() so you modify the correct retry handler.src/renderer/components/AgentErrorModal.tsx (1)
267-279:⚠️ Potential issue | 🟠 MajorExpired parsed reset time incorrectly triggers a fresh fallback wait.
When
error.rateLimitResetAtexists but is now in the past (line 268 fails), the code falls through and computes a fresh fallback fromerror.timestamp + fallbackWaitMs. This turns a rate limit that is ready to retry into a brand-new multi-hour delay.If the parsed reset time is expired, the user should be able to retry immediately rather than waiting again.
♻️ Suggested fix
// Exact parsed reset time is always preferred - if (error.rateLimitResetAt && error.rateLimitResetAt > Date.now()) { - return error.rateLimitResetAt; + if (error.rateLimitResetAt) { + // If parsed time is in the future, use it + if (error.rateLimitResetAt > Date.now()) { + return error.rateLimitResetAt; + } + // If parsed time exists but is expired, allow immediate retry (no countdown) + return null; } // If no fallback is configured or available, don't show countdown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentErrorModal.tsx` around lines 267 - 279, The current logic in the useMemo callback in AgentErrorModal incorrectly falls through to a long fallback when error.rateLimitResetAt exists but is in the past; change the branch so that if error.rateLimitResetAt is present and <= Date.now() you return null (no countdown / allow immediate retry) instead of computing the fallback. Specifically update the block that checks error.rateLimitResetAt to return null for expired parsed reset times, keeping the existing fallback calculation using autoRetrySettings.fallbackHours, fallbackWaitMs and error.timestamp only when rateLimitResetAt is absent.
🧹 Nitpick comments (1)
src/renderer/components/AgentErrorModal.tsx (1)
220-231: Consider preventing double-invocation if user clicks "Try Again" during countdown.The retry action button is still clickable while the countdown runs. If the user clicks it manually just before
onCompletefires,retryAction.onClick()could be called twice in quick succession.Consider disabling the retry button while countdown is active, or using a guard in
handleCountdownCompleteto check if retry was already triggered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentErrorModal.tsx` around lines 220 - 231, The retry path can be invoked twice because handleCountdownComplete calls retryAction.onClick() while the retry button remains clickable; add a guard or disable the button during the countdown by introducing a local state flag (e.g., isRetrying or retryInvoked) and update it when the user clicks the retry button or when handleCountdownComplete runs, then use that flag to (a) short-circuit handleCountdownComplete (return early if already invoked) and (b) pass it to the retry button to disable clicks while the countdown is active; reference retryAction, handleCountdownComplete and recoveryActions when locating where to set and check this flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/components/AgentErrorModal.tsx`:
- Around line 267-279: The current logic in the useMemo callback in
AgentErrorModal incorrectly falls through to a long fallback when
error.rateLimitResetAt exists but is in the past; change the branch so that if
error.rateLimitResetAt is present and <= Date.now() you return null (no
countdown / allow immediate retry) instead of computing the fallback.
Specifically update the block that checks error.rateLimitResetAt to return null
for expired parsed reset times, keeping the existing fallback calculation using
autoRetrySettings.fallbackHours, fallbackWaitMs and error.timestamp only when
rateLimitResetAt is absent.
In `@src/renderer/stores/agentStore.ts`:
- Around line 226-229: The retryAfterError guard rejects image-only requests
because it requires lastUserLog.text; update retryAfterError to treat image-only
queued messages the same as processQueuedItem by falling back to the synthesized
prompt: if lastUserLog.text is falsy but the log represents an image-only upload
(or the queue item lacks text), use DEFAULT_IMAGE_ONLY_PROMPT before returning,
or check for the same image-only flag/condition used by processQueuedItem so
rate-limited screenshot uploads can be retried; reference retryAfterError,
processQueuedItem, lastUserLog, and DEFAULT_IMAGE_ONLY_PROMPT to implement the
parity.
- Around line 282-288: The retry callback currently swallows failures (only
console.error); update the catch block for get().processQueuedItem(sessionId,
queuedItem, deps) to report the error to Sentry (e.g., call
Sentry.captureException(err)) and then restore the session's error state so the
user sees the error modal again by reusing the store's existing error setter
(the same method/property used earlier to clear the error state), e.g., call
get().setSessionError(sessionId, err) or the appropriate restoreError method
after capturing the exception; keep references to processQueuedItem and get() so
you modify the correct retry handler.
---
Nitpick comments:
In `@src/renderer/components/AgentErrorModal.tsx`:
- Around line 220-231: The retry path can be invoked twice because
handleCountdownComplete calls retryAction.onClick() while the retry button
remains clickable; add a guard or disable the button during the countdown by
introducing a local state flag (e.g., isRetrying or retryInvoked) and update it
when the user clicks the retry button or when handleCountdownComplete runs, then
use that flag to (a) short-circuit handleCountdownComplete (return early if
already invoked) and (b) pass it to the retry button to disable clicks while the
countdown is active; reference retryAction, handleCountdownComplete and
recoveryActions when locating where to set and check this flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8a386c9-7a9b-4eea-bc1b-6732fd7da95c
📒 Files selected for processing (6)
src/__tests__/main/parsers/rate-limit-event.test.tssrc/main/agents/definitions.tssrc/main/parsers/claude-output-parser.tssrc/main/parsers/error-patterns.tssrc/renderer/components/AgentErrorModal.tsxsrc/renderer/stores/agentStore.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/main/parsers/rate-limit-event.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/parsers/error-patterns.ts
Collaborator
|
Hey @ProaFilippi — thanks for putting this together, and congrats on your first public PR! No need to be afraid of looking silly — this is a real feature solving a real pain point, and we appreciate you taking the time to build it out. We've reviewed the implementation and think the direction is solid. There are some things we'd like to tighten up before merging, but nothing that changes the core idea. Here's a summary: What we like:
What we'd like to refine:
We're happy to work with you to groom this into production-grade quality. We'll be making some refinements on our side and will keep you posted. Thanks again for the contribution — looking forward to shipping this! |
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
2 participants
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 is my first PR in a public project.
I'm afraid of looking silly, and also afraid of breaking things.
--- That said --
This feature direct address on need of my.
I want to know how long will take to Claude code to reset its limits, and also to maestro to auto retry after this period.
This is a initial version of it.
I think that the full feature needs to be delivered w/ settings around it. Like Enable/Disable auto retry. and a Fallback auto retry when the parse of the timings get wrong.
So, I dont know if I should send a PR with an MVP of a feature I'm using intensely, of if I should wait.
Summary by CodeRabbit
New Features
Bug Fixes
Tests