Skip to content

OpenConceptLab/ocl_issues#2483 | Throttling Error UI#12

Open
snyaggarwal wants to merge 2 commits intomainfrom
issues#2483
Open

OpenConceptLab/ocl_issues#2483 | Throttling Error UI#12
snyaggarwal wants to merge 2 commits intomainfrom
issues#2483

Conversation

@snyaggarwal
Copy link
Copy Markdown
Contributor

Linked Issue

Closes OpenConceptLab/ocl_issues#2483

Screenshots are in ticket

@snyaggarwal snyaggarwal requested a review from paynejd April 21, 2026 07:04
Copy link
Copy Markdown
Member

@paynejd paynejd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — two bugs to fix before merge

1. Translation interpolation mismatch (es/zh)

The English limit_hit strings are plain text, but the Spanish and Chinese translations include {{minuteRemaining}} and {{dayRemaining}} interpolation placeholders that are never passed from the component. getLimitMessage in ThrottlingError.jsx calls t('errors.throttled.limit_hit.minute') without any interpolation values.

As-is, Spanish and Chinese users will see literal {{minuteRemaining}} in the UI.

Fix: either remove the {{...}} placeholders from es and zh to match en, or pass the remaining values through to the t() call (which would require ThrottlingError to receive minuteRemaining/dayRemaining as props).

2. Double-processing of throttling details in App.jsx

In App.jsx, the onThrottle callback does:

const unsubscribe = APIService.onThrottle(details => {
  setThrottlingError(getThrottlingDetails(details.response || details))
})

But notifyThrottlingListeners already calls getThrottlingDetails internally and passes the processed result to the listener. So details is already a throttling-details object (with retryAfter, limitType, etc.) — it has no .headers property. Wrapping it in getThrottlingDetails again extracts headers from a plain object that doesn't have any, so everything falls back to defaults (retryAfter: 0, limitType: 'unknown').

Fix: use the details directly:

const unsubscribe = APIService.onThrottle(details => {
  setThrottlingError(details)
})

Everything else looks good — architecture is clean, overlay-not-route is the right UX call, and the i18n coverage is solid. These two should be quick fixes.

Copy link
Copy Markdown
Member

@paynejd paynejd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both issues from my previous review are addressed:

  1. Translation interpolation mismatch (es/zh) — fixed; all three locales now use plain text without {{...}} placeholders.
  2. Double-processing in App.jsx — fixed; setThrottlingError(details) is now called directly so retryAfter / limitType flow through correctly.

Ticket acceptance criteria are all met. A few minor non-blocking observations for a possible follow-up:

  • Dead !== 429 branch in fetchToggles — 429s never reach the .then handler since APIService converts them to a never-resolving promise.
  • ThrottlingError countdown effect recreates its interval every tick (depends on secondsLeft); pinning to [retryAfter] would be cleaner.
  • createPendingRequest() means .finally() cleanup in callers won't fire on 429 — fine in practice since the overlay covers the page.
  • Overlay has no role="dialog" / aria-live — minor a11y polish.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mapper handling API throttling

2 participants