Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a server-side provider limits synchronization system, replacing the previous client-side auto-refresh logic. It includes a new background scheduler, database caching for provider limits, and updated API endpoints to handle both manual and scheduled syncs. Documentation and environment variables have been updated to reflect these changes. Feedback suggests simplifying redundant object assignments in the credential refresh logic and improving error handling when processing settled promises during bulk synchronization.
| connection: { | ||
| ...connection, | ||
| ...updateData, | ||
| providerSpecificData: | ||
| (updateData.providerSpecificData as JsonRecord | undefined) || | ||
| connection.providerSpecificData, | ||
| }, |
There was a problem hiding this comment.
The explicit assignment of providerSpecificData is redundant here. Since updateData is spread after connection, if updateData.providerSpecificData is defined (which happens in the logic above), it will naturally overwrite the value from connection. If it is undefined, the value from connection is preserved. Removing this line simplifies the object construction while maintaining the same logic.
connection: {
...connection,
...updateData,
},| const reason = result.reason as { message?: string } | undefined; | ||
| errors[connectionId] = reason?.message || "Failed to refresh provider limits"; |
There was a problem hiding this comment.
When handling rejected promises from Promise.allSettled, the reason can be any type (e.g., a string or a custom object), not necessarily an object with a message property. Using instanceof Error or a robust string fallback ensures that the errors map is always populated with a meaningful string, preventing potential undefined values in the UI.
| const reason = result.reason as { message?: string } | undefined; | |
| errors[connectionId] = reason?.message || "Failed to refresh provider limits"; | |
| const reason = result.reason; | |
| errors[connectionId] = reason instanceof Error ? reason.message : String(reason || "Failed to refresh provider limits"); |
rdself
changed the title
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review - Additional Issues Found
I've identified a few additional issues beyond the existing comments:
-
Scheduler runs immediately on server startup - The scheduler waits 5 seconds (STARTUP_DELAY_MS) but runs on the first request that triggers the import, not on a fixed schedule. This could cause delays.
-
No upper bound on sync interval - The env var validation allows any positive number, meaning extremely large values (e.g., 525600 minutes = 1 year) are technically accepted.
Code Review SummaryStatus: 2 Existing Issues | Recommendation: Address before merge Overview
Existing Issues (from other reviewers)The following issues have already been identified by other reviewers:
Other Observations (not in diff)Issues found in unchanged code or patterns that should be considered:
Files Reviewed (6 files)
Overall Assessment: The PR implements the caching layer correctly and addresses the original issue of avoiding live upstream refreshes on page load. The existing comments from other reviewers identify actionable improvements. The additional observations are minor and relate to edge cases that don't block merge. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a server-scheduled, SQLite-backed cache for Provider Limits so that opening the dashboard page no longer triggers a full live quota refresh burst against upstream provider limits endpoints.
Changes:
- Added a
key_value-backed Provider Limits cache plus a shared live-fetch + persist usage service. - Added a Node-instrumentation-started scheduler to auto-sync Provider Limits on a configurable cadence (
PROVIDER_LIMITS_SYNC_INTERVAL_MINUTES, default 70m). - Updated dashboard + APIs to hydrate from cached data on page load and keep manual refresh behaviors.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/shared/services/providerLimitsSyncScheduler.ts |
New periodic sync scheduler with startup delay and last-run alignment. |
src/lib/usage/providerLimits.ts |
New shared service for fetching live provider limits, persisting cache, and bulk sync. |
src/lib/db/providerLimits.ts |
New DB helpers for reading/writing Provider Limits cache entries in key_value. |
src/lib/localDb.ts |
Re-export Provider Limits cache DB functions/types via localDb facade. |
src/instrumentation-node.ts |
Starts the Provider Limits sync scheduler at Node startup. |
src/app/api/usage/provider-limits/route.ts |
New endpoints to read cached limits and manually sync all. |
src/app/api/usage/[connectionId]/route.ts |
Refactored to use the shared Provider Limits live-fetch + persist logic. |
src/app/(dashboard)/dashboard/usage/components/ProviderLimits/index.tsx |
Page now hydrates from cached limits on mount; manual refresh all uses the new bulk endpoint. |
README.md |
Updated feature bullet to reflect cached/scheduled Provider Limits tracking. |
docs/USER_GUIDE.md |
Documented PROVIDER_LIMITS_SYNC_INTERVAL_MINUTES in env var table. |
.env.example |
Added PROVIDER_LIMITS_SYNC_INTERVAL_MINUTES=70. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (elapsedMs < intervalMs) { | ||
| initialDelayMs = Math.max(intervalMs - elapsedMs, STARTUP_DELAY_MS); |
There was a problem hiding this comment.
elapsedMs can go negative if lastAutoSyncAt is in the future (clock skew / bad setting value), which makes initialDelayMs larger than intervalMs (because intervalMs - elapsedMs becomes intervalMs + |elapsedMs|). Consider guarding against future timestamps (e.g., treat negative elapsed as 0 or fall back to STARTUP_DELAY_MS) so a bad timestamp can’t delay sync for hours.
| if (elapsedMs < intervalMs) { | |
| initialDelayMs = Math.max(intervalMs - elapsedMs, STARTUP_DELAY_MS); | |
| const safeElapsedMs = Math.max(0, elapsedMs); | |
| if (safeElapsedMs < intervalMs) { | |
| initialDelayMs = Math.max(intervalMs - safeElapsedMs, STARTUP_DELAY_MS); |
| `[ProviderLimitsSync] Cycle complete: ${result.succeeded}/${result.total} synced in ${Date.now() - start}ms` | ||
| ); | ||
| } catch (error) { | ||
| console.warn("[ProviderLimitsSync] Cycle failed:", (error as Error).message); |
There was a problem hiding this comment.
The catch block logs (error as Error).message, which can be undefined for non-Error throws (and it drops stack/context). Prefer logging the error object (or error instanceof Error ? error.message : String(error)) to preserve useful diagnostics.
| console.warn("[ProviderLimitsSync] Cycle failed:", (error as Error).message); | |
| console.warn( | |
| "[ProviderLimitsSync] Cycle failed:", | |
| error instanceof Error ? error : String(error) | |
| ); |
| const chunk = connections.slice(i, i + concurrency); | ||
| const results = await Promise.allSettled( | ||
| chunk.map(async (connection) => { | ||
| const { usage } = await fetchLiveProviderLimits(connection.id); |
There was a problem hiding this comment.
syncAllProviderLimits() iterates over connections from getProviderConnections(), but each task calls fetchLiveProviderLimits(connection.id) which re-queries the DB via getProviderConnectionById for every connection. This is an avoidable N+1 DB pattern; consider adding an overload that accepts the already-loaded connection object (or splitting out an internal fetchLiveProviderLimitsForConnection(connection)), and only hit the DB when you truly need a fresh row.
| const { usage } = await fetchLiveProviderLimits(connection.id); | |
| const { usage } = await fetchLiveProviderLimits(connection); |
| export function getProviderLimitsSyncIntervalMinutes(): number { | ||
| const raw = Number.parseInt(process.env.PROVIDER_LIMITS_SYNC_INTERVAL_MINUTES ?? "", 10); | ||
| return Number.isFinite(raw) && raw > 0 ? raw : DEFAULT_PROVIDER_LIMITS_SYNC_INTERVAL_MINUTES; | ||
| } | ||
|
|
||
| export function getProviderLimitsSyncIntervalMs(): number { | ||
| return getProviderLimitsSyncIntervalMinutes() * 60 * 1000; | ||
| } |
There was a problem hiding this comment.
getProviderLimitsSyncIntervalMinutes() and the sync/cache persistence logic are new behavior but there are no unit tests covering interval parsing (unset/invalid/zero/negative), cached read/write normalization, and syncAllProviderLimits() error aggregation. Given the repo has unit tests for other src/lib/usage/* modules, adding a focused tests/unit/provider-limits.test.mjs would help prevent regressions.
| plan: cached.plan || null, | ||
| message: cached.message || null, |
There was a problem hiding this comment.
plan: cached.plan || null will coerce falsy-but-valid plan values (e.g., 0, false, or empty string) to null. If you only want to default when the field is missing, use nullish coalescing (cached.plan ?? null). Same applies to cached.message if you want to preserve empty strings.
| plan: cached.plan || null, | |
| message: cached.message || null, | |
| plan: cached.plan ?? null, | |
| message: cached.message ?? null, |
| const row = db | ||
| .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") | ||
| .get(PROVIDER_LIMITS_CACHE_NAMESPACE, connectionId) as KeyValueRow | undefined; | ||
| if (!row?.value) return null; | ||
| return normalizeCacheEntry(parseJson(row.value)); |
There was a problem hiding this comment.
The SELECT only fetches value, but the result is cast to KeyValueRow (which includes key). This type mismatch makes the code harder to reason about and can mask mistakes. Consider introducing a ValueRow type ({ value: string }) for this query (or select both key, value).
|
Merged manually into |
…(design phase 3, C3) The shared DataTable hardcoded its colors with inline rgba and two dead var() fallbacks — var(--bg-table-header, rgba(15,15,25,0.95)) and var(--text-secondary, #888) — that never resolved, so the table was effectively always-dark and broken in light theme. Introduce a --table-* token set whose DARK values exactly equal the old hardcoded rgba (header bg, zebra, row hover, cell border) so dark stays byte-identical, and whose light values fix the light theme. Header border → --color-border (which is rgba(255,255,255,0.08) in dark = the exact old value); secondary text → --color-text-muted; the selected-row accent tint is kept (theme-neutral).⚠️ Wants a visual pass before merge — light-theme tables + the #888→muted text shift. logTableStyles.ts + the legacy Ant table rules are left untouched (separate, lower priority). See design.md.
…ote-only replace CodeQL js/incomplete-sanitization (#888): the hand-rolled replace only escaped double quotes, so a backslash in the fixture would have produced a malformed YAML scalar. JSON.stringify covers every escape the double-quoted YAML scalar needs.
…ote-only replace (#11942) CodeQL js/incomplete-sanitization (alert #888 on #11929): the hand-rolled replace only escaped double quotes, so a backslash in the fixture would have produced a malformed YAML scalar. JSON.stringify covers every escape the double-quoted YAML scalar needs. Test-only change (7/7 pass).
…ote-only replace CodeQL js/incomplete-sanitization (cheaper-inference#888): the hand-rolled replace only escaped double quotes, so a backslash in the fixture would have produced a malformed YAML scalar. JSON.stringify covers every escape the double-quoted YAML scalar needs.
…ote-only replace (cheaper-inference#11942) CodeQL js/incomplete-sanitization (alert cheaper-inference#888 on cheaper-inference#11929): the hand-rolled replace only escaped double quotes, so a backslash in the fixture would have produced a malformed YAML scalar. JSON.stringify covers every escape the double-quoted YAML scalar needs. Test-only change (7/7 pass).
What changed
This PR stops Provider Limits from doing a full live refresh every time the page opens.
Instead, Provider Limits now:
PROVIDER_LIMITS_SYNC_INTERVAL_MINUTESWhy this changed
The previous implementation triggered
/api/usage/[connectionId]for every supported account on component mount. That meant simply opening the Provider Limits page caused a live quota sync burst against upstream providers.Some providers/channels enforce very strict RPM limits on quota or limits endpoints. When users open this page repeatedly, those quota fetches can hit upstream 429s very easily even though regular model traffic is otherwise fine.
The root cause was that the dashboard only had an in-memory front-end state for Provider Limits, so it had to refresh live on every mount to show anything.
Implementation details
key_value/api/usage/provider-limits.env.example,README.md, anddocs/USER_GUIDE.mdImpact
Validation
npx eslint 'src/lib/db/providerLimits.ts' 'src/lib/usage/providerLimits.ts' 'src/shared/services/providerLimitsSyncScheduler.ts' 'src/app/api/usage/[connectionId]/route.ts' 'src/app/api/usage/provider-limits/route.ts' 'src/app/(dashboard)/dashboard/usage/components/ProviderLimits/index.tsx' 'src/instrumentation-node.ts'node --import tsx/esm --test tests/unit/copilot-usage.test.mjs(fails on a pre-existing GitHub Copilot quota assertion unrelated to this diff: expectedremainingPercentage60, got 100)npm run typecheck:core(blocked by a pre-existing error inopen-sse/handlers/responseTranslator.ts:410whereopenaiResponse.choicesis typed asunknown)