Add Codex OAuth limit estimates and LiteLLM pricing sync#226
Add Codex OAuth limit estimates and LiteLLM pricing sync#226WenjieZhao1 wants to merge 1 commit into
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 pricing synchronization from LiteLLM and adds a new endpoint to estimate OAuth limits for Codex identities based on rolling windows (5h, 7d) and model pricing. The frontend has been updated to display sync status, trigger manual LiteLLM syncs, and show OAuth limit estimates on credentials. Key feedback from the review includes: (1) fixing a database-agnostic migration issue where raw SQL with DATETIME will fail on PostgreSQL by using GORM's Migrator().AddColumn, (2) centralizing AbortController management in the useOAuthLimitEstimates React hook to prevent race conditions, (3) adding a defensive nil check on task in oauth_limit_estimates.go to avoid nil pointer dereferences, and (4) removing unused write-only fields (mu and running) from PricingSyncRunner.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| columns := []struct { | ||
| name string | ||
| sql string | ||
| }{ | ||
| {name: "source", sql: "ALTER TABLE model_price_settings ADD COLUMN source TEXT NOT NULL DEFAULT 'manual'"}, | ||
| {name: "source_url", sql: "ALTER TABLE model_price_settings ADD COLUMN source_url TEXT"}, | ||
| {name: "synced_at", sql: "ALTER TABLE model_price_settings ADD COLUMN synced_at DATETIME"}, | ||
| } | ||
|
|
||
| for _, column := range columns { | ||
| if tx.Migrator().HasColumn(&entities.ModelPriceSetting{}, column.name) { | ||
| continue | ||
| } | ||
| if err := tx.Exec(column.sql).Error; err != nil { | ||
| return fmt.Errorf("add model_price_settings.%s column: %w", column.name, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Using raw SQL ALTER TABLE with DATETIME type is not database-agnostic and will fail on PostgreSQL, which does not support the DATETIME type (it uses TIMESTAMP instead). Use GORM's built-in Migrator().AddColumn to perform database-agnostic schema migrations safely.
| columns := []struct { | |
| name string | |
| sql string | |
| }{ | |
| {name: "source", sql: "ALTER TABLE model_price_settings ADD COLUMN source TEXT NOT NULL DEFAULT 'manual'"}, | |
| {name: "source_url", sql: "ALTER TABLE model_price_settings ADD COLUMN source_url TEXT"}, | |
| {name: "synced_at", sql: "ALTER TABLE model_price_settings ADD COLUMN synced_at DATETIME"}, | |
| } | |
| for _, column := range columns { | |
| if tx.Migrator().HasColumn(&entities.ModelPriceSetting{}, column.name) { | |
| continue | |
| } | |
| if err := tx.Exec(column.sql).Error; err != nil { | |
| return fmt.Errorf("add model_price_settings.%s column: %w", column.name, err) | |
| } | |
| } | |
| columns := []string{"Source", "SourceURL", "SyncedAt"} | |
| for _, column := range columns { | |
| if tx.Migrator().HasColumn(&entities.ModelPriceSetting{}, column) { | |
| continue | |
| } | |
| if err := tx.Migrator().AddColumn(&entities.ModelPriceSetting{}, column); err != nil { | |
| return fmt.Errorf("add model_price_settings.%s column: %w", column, err) | |
| } | |
| } |
| const load = useCallback(async (signal?: AbortSignal) => { | ||
| if (!enabled) { | ||
| return | ||
| } | ||
| if (authIndexes.length === 0) { | ||
| setEstimatesByAuthIndex({}) | ||
| setLoading(false) | ||
| setError('') | ||
| return | ||
| } | ||
|
|
||
| setLoading(true) | ||
| setError('') | ||
| try { | ||
| const response = await fetchUsageOAuthLimitEstimates(authIndexes, signal) | ||
| if (signal?.aborted) { | ||
| return | ||
| } | ||
| setEstimatesByAuthIndex(() => { | ||
| const requested = new Set(authIndexes) | ||
| const next: Record<string, UsageOAuthLimitEstimateItem> = {} | ||
| for (const item of response.items ?? []) { | ||
| if (requested.has(item.auth_index)) { | ||
| next[item.auth_index] = item | ||
| } | ||
| } | ||
| return next | ||
| }) | ||
| } catch (nextError) { | ||
| if (signal?.aborted) { | ||
| return | ||
| } | ||
| if (nextError instanceof ApiError && nextError.status === 401) { | ||
| onAuthRequired?.() | ||
| return | ||
| } | ||
| setError(oauthLimitEstimateErrorMessage(nextError)) | ||
| } finally { | ||
| if (!signal?.aborted) { | ||
| setLoading(false) | ||
| } | ||
| } | ||
| }, [authIndexes, enabled, onAuthRequired]) | ||
|
|
||
| useEffect(() => { | ||
| if (!enabled) { | ||
| requestControllerRef.current?.abort() | ||
| requestControllerRef.current = null | ||
| setLoading(false) | ||
| return | ||
| } | ||
| requestControllerRef.current?.abort() | ||
| const controller = new AbortController() | ||
| requestControllerRef.current = controller | ||
| void load(controller.signal).finally(() => { | ||
| if (requestControllerRef.current === controller) { | ||
| requestControllerRef.current = null | ||
| } | ||
| }) | ||
| return () => { | ||
| controller.abort() | ||
| } | ||
| }, [enabled, load]) | ||
|
|
||
| const refresh = useCallback(async () => { | ||
| await load() | ||
| }, [load]) |
There was a problem hiding this comment.
Centralize the AbortController management inside the load function. This prevents potential race conditions where a manual refresh call (which currently doesn't use or manage an abort signal) could run concurrently with automatic loads and overwrite the state with stale data. It also simplifies the useEffect and allows callers to safely await the refresh promise.
const load = useCallback(async () => {
if (!enabled) {
return
}
if (authIndexes.length === 0) {
setEstimatesByAuthIndex({})
setLoading(false)
setError('')
return
}
requestControllerRef.current?.abort()
const controller = new AbortController()
requestControllerRef.current = controller
setLoading(true)
setError('')
try {
const response = await fetchUsageOAuthLimitEstimates(authIndexes, controller.signal)
if (controller.signal.aborted) {
return
}
setEstimatesByAuthIndex(() => {
const requested = new Set(authIndexes)
const next: Record<string, UsageOAuthLimitEstimateItem> = {}
for (const item of response.items ?? []) {
if (requested.has(item.auth_index)) {
next[item.auth_index] = item
}
}
return next
})
} catch (nextError) {
if (controller.signal.aborted) {
return
}
if (nextError instanceof ApiError && nextError.status === 401) {
onAuthRequired?.()
return
}
setError(oauthLimitEstimateErrorMessage(nextError))
} finally {
if (!controller.signal.aborted) {
setLoading(false)
if (requestControllerRef.current === controller) {
requestControllerRef.current = null
}
}
}
}, [authIndexes, enabled, onAuthRequired])
useEffect(() => {
void load()
return () => {
requestControllerRef.current?.abort()
}
}, [load])
const refresh = useCallback(async () => {
await load()
}, [load])|
|
||
| for _, authIndex := range authIndexes { | ||
| task, ok := s.refreshTasks[authIndex] | ||
| if !ok || task.Status != RefreshTaskStatusCompleted || task.Quota == nil { |
There was a problem hiding this comment.
Defensive programming: check if task is nil before accessing its fields to prevent potential nil pointer dereference panics.
| if !ok || task.Status != RefreshTaskStatusCompleted || task.Quota == nil { | |
| if !ok || task == nil || task.Status != RefreshTaskStatusCompleted || task.Quota == nil { |
References
- Ensure pointer receivers in Go are checked for nil before accessing their fields or locking their mutexes to prevent nil pointer dereference panics.
| mu sync.Mutex | ||
| running bool |
There was a problem hiding this comment.
The mu and running fields in PricingSyncRunner are write-only and never read anywhere in the codebase. Removing them simplifies the runner and improves maintainability.
syncer PricingSyncer
now func() time.Time
sleep func(context.Context, time.Duration) bool
|
This duplicates existing main-branch functionality. Auth Files already has quota Current/Estimated estimation, and Settings already has manual model price sync. Please reuse/extend those existing flows instead of adding parallel Codex-only estimate APIs/UI and a second pricing sync path. |
|
Thanks for the review. I may not have described the intent clearly enough. The existing Auth Files quota Current/Estimated flow shows the provider quota percentage and can estimate usage from quota window data. What I was trying to add is a more specific measurement: estimating the approximate USD value of each Codex OAuth account’s 5h and weekly quota, based on Keeper’s local SQLite usage records plus local model prices. There has been a lot of discussion around Codex quotas for ChatGPT Plus and Team accounts. In practice, the quota is hard to quantify precisely, and it appears that even different Team accounts may have different effective limits. This feature is meant to make that measurable from real local usage data instead of guessing from plan names. For each Codex OAuth account, it uses:
Then it infers:
So the goal is not to duplicate the existing quota bar, but to answer: “roughly how many tokens / dollars of model usage does this specific account’s 5h and weekly quota represent?” |
|
Thanks for the PR and for clarifying the intent. The idea itself is useful: estimating the approximate token/USD value of a quota window can help users better understand what each account’s 5h / weekly quota means in practice. However, I do not think this should be added as a parallel flow in its current form. First, the quota estimate feature overlaps with the existing Auth Files quota Current/Estimated flow. With this PR, the page would show two separate quota estimation concepts for the same account/window, which is confusing from a product perspective. This should be merged into the existing quota estimate model/UI instead of adding a separate Codex-only estimate API, hook, and panel. The current UI placement also makes each Auth Files row much taller and less predictable, since the estimate cards are always rendered under the quota bars. This becomes especially visible for Codex Pro accounts, where the quota area can already render four quota bars. Adding two more estimate cards below those bars would make a single row very tall and harder to scan. A better interaction would be to keep the row compact and show these richer token/USD details in a popover or modal when the user clicks the quota bar. Also, the new estimate logic is currently Codex-specific. Existing quota estimation has already been optimized around 5h / weekly quota windows more generally, so if we extend this feature, it should fit the existing quota system and be designed in a way that can support other credential/provider types where applicable. Second, the LiteLLM pricing sync should be integrated with the existing manual pricing sync flow instead of introducing a second sync path and a second source-specific workflow. Price sync should have one product model and one UI flow, with different sources handled as options within that flow if needed. So I think the next step should be to re-scope this PR around extending the existing Auth Files quota estimation and existing pricing sync mechanisms, rather than adding parallel systems. |
4c8b876 to
4b05e27
Compare
|
Updated this PR based on the review feedback and force-pushed the re-scoped implementation. What changed:
Verification:
|
|
Thanks for the contribution and for the follow-up updates. I pulled your branch locally, ran it, and reviewed the feature from an end-user perspective. I appreciate the effort, but I do not think this PR is ready to be accepted. A feature PR should be reviewed and polished by the contributor before submission, including actually running and using the feature in the application. During this review, I had to repeatedly pull the branch to inspect both the code and the implementation. Much of the feedback was about product flow, integration with existing UI/business logic, and feature polish rather than small review fixes. Also, a PR should ideally focus on one feature or one coherent change. Unpolished PRs that require maintainers to repeatedly guide the feature design and user experience will not be accepted. Contributions are welcome, but please make sure the feature is complete, integrated with the existing product model, personally tested, and scoped to a clear single purpose before opening or updating the PR. |
2 participants
Summary
Details
POST /api/v1/usage/oauth-limit-estimates.GET /api/v1/pricing/syncandPOST /api/v1/pricing/sync.model_price_settingswithsource,source_url, andsynced_atmetadata.Testing
go test ./cmd/... ./internal/...npm --prefix ./web run testnpm --prefix ./web run lintnpm --prefix ./web run typechecknpm --prefix ./web run build