[FEAT] Budgets & Analytics#42
Merged
Merged
Conversation
This commit introduces a comprehensive budget and analytics system with: **Backend:** - New database migrations for budgets, budget_assignments, usage_events, and model_pricing_overrides tables - Budget types with support for pooled/per-user kinds, flexible intervals (daily/weekly/monthly/total), reset strategies (calendar/rolling/anchored), and exceed actions (block/warn/allow) - Usage event tracking with token counts and costs - Admin routes for budget CRUD, assignments, and analytics queries (grouped by model/user/team/day/day_model) - Public routes for user budget status and personal analytics - Model pricing overrides stored in JSONB for custom per-model pricing - Budget enforcement integration in model listing (budget_blocked flag) **Frontend:** - Budget management page with create/edit/delete, team/user assignments - Analytics dashboard with overview/trends/explore tabs, stacked bar charts, area charts, and horizontal bar charts - Reusable chart components (StackedAnalyticsChart, HorizontalAnalyticsChart) - Budget indicator in sidebar showing remaining budget and usage progress bar - Model selector integration showing budget-blocked models - Profile page tabs for personal analytics - Model editor pricing override section **Version:** Bumped from 0.1.8 to 0.2.0
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Add PostgreSQL advisory lock for budget checks in streaming to prevent race conditions - Batch budget spend queries using UNNEST instead of N+1 queries per budget - Calculate usage costs locally from pricing overrides instead of trusting provider-reported costs - Fix budget assignment deletion to require budget_id for safety - Fix analytics dashboard cost change percentage display logic - Escape LIKE patterns in budget search to prevent injection
- Run `cargo update` to refresh all dependencies to latest compatible versions - Upgrade omniference from 0.1.8 to 0.2 (major version bump)
- Replace custom date controls in `AnalyticsDashboard.vue` with `AnalyticsDateFilter` - Add logic for date preset selections and custom range handling in `AnalyticsDateFilter.vue` - Enhance stacked bar chart tooltips and hover interactions in `StackedAnalyticsChart.vue` - Add i18n translations for new analytics presets and sections in English and German - Update migration scripts to include additional analytics translations
- Implement new sections for models, API keys, and users with separate cost summaries and charts - Add sparklines, icons, and detailed breakdowns for trending items in models and users - Refactor layout and component usage for improved hierarchy and readability - Enhance spend-over-time chart and add placeholders for API key trends - Add i18n updates for section titles - Update migration script for revised translations
…d i18n support - Replace native `<select>` elements with `ShadSelect` for metric, grouping, top N, and rollup options - Add dynamic labels, rollup options, and improved table layout for explore data - Extend i18n translations for additional analytics keys in English and German - Update migration scripts for new translations in `i18n_translations` table - Refactor chart and table logic for user, model, and rollup aggregations
- Merge multiple migration scripts for budgets and analytics into a single file - Refactor i18n translation updates for better organization and conflict handling - Add support for model icons in analytics charts with edge case handling in frontend
- Add support for filtering analytics by individual user in backend and UI - Extend admin analytics dashboard with user selector for targeted analysis - Update data stores and API fetch logic to include user-specific analytics - Add new button in user settings for quick access to per-user analytics - Implement i18n translations and optimize frontend components for user-based filtering
…signed frontend for budget management - Backend changes: - Add `BudgetResetRequest` and `BudgetResetEventResponse` structures - Implement budget reset logic and latest resets retrieval for users/teams - Introduce `user_overview` and `team_overview` endpoints for detailed summaries - Add SQL queries for reset events, spending aggregation, and budget usage tracking - Frontend changes: - Redesign budget management interface with tabs for users, teams, budgets, and history - Implement budget reset actions with expanded tables for user/team details - Add search filters, enhanced UI components, and i18n support for new features
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds budgeting, usage analytics, and model pricing override flows across the database, backend APIs, frontend settings UI, and chat/model selection. It also updates crate metadata, the build image, and provider model discovery behavior. ChangesBudgets, Analytics, and Pricing
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/stores/chatStore.ts (1)
759-773: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWrap the
case 'done'body in braces before declaringconst budgetStore. Biome’s switch-scope rule flags this unbraced lexical declaration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/stores/chatStore.ts` around lines 759 - 773, The `case 'done'` block in `chatStore.ts` contains a lexical declaration (`const budgetStore`) without its own scope, which violates the switch-scope rule. Wrap the entire `case 'done'` body in braces in the switch handling around the `done` branch, keeping the existing `Object.assign`, `this.isStreaming`, `chatIndex`/`chat` updates, and `useBudgetStore()` logic inside that scoped block.Source: Linters/SAST tools
src/types/teams/repository.rs (1)
185-201: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
update_budgettransactional
teams.budget_idis only used in team-facing responses; enforcement goes throughbudget_assignments. If the second write fails after theteamsrow update commits, the legacy column and assignment table diverge, so the team can display one budget while enforcement uses another. Wrap both operations in one transaction or accept a shared executor inreplace_team_budget_from_legacy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/teams/repository.rs` around lines 185 - 201, The update_budget method currently performs the teams row update and Budget::replace_team_budget_from_legacy as separate operations, which can leave the legacy column and budget_assignments out of sync if the second step fails. Update teams::repository::update_budget to run both writes through the same transaction, or refactor Budget::replace_team_budget_from_legacy to accept a shared executor so both steps can be executed atomically. Use the existing update_budget and replace_team_budget_from_legacy symbols to keep the fix localized.
🟡 Minor comments (7)
frontend/pages/settings/models/[id].vue-840-884 (1)
840-884: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConstrain price inputs to non-negative values.
The save path should validate too, but adding
min="0"prevents accidental negative pricing from the UI.Proposed fix
- <ShadInput v-model="pricingForm.input" type="number" step="0.000001" /> + <ShadInput v-model="pricingForm.input" type="number" min="0" step="0.000001" /> @@ - <ShadInput v-model="pricingForm.output" type="number" step="0.000001" /> + <ShadInput v-model="pricingForm.output" type="number" min="0" step="0.000001" /> @@ - <ShadInput v-model="pricingForm.reasoning" type="number" step="0.000001" /> + <ShadInput v-model="pricingForm.reasoning" type="number" min="0" step="0.000001" /> @@ - <ShadInput v-model="pricingForm.cache_read" type="number" step="0.000001" /> + <ShadInput v-model="pricingForm.cache_read" type="number" min="0" step="0.000001" /> @@ - <ShadInput v-model="pricingForm.cache_write" type="number" step="0.000001" /> + <ShadInput v-model="pricingForm.cache_write" type="number" min="0" step="0.000001" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/settings/models/`[id].vue around lines 840 - 884, The pricing override form in the settings models editor allows negative values because the ShadInput fields for pricingForm.input, pricingForm.output, pricingForm.reasoning, pricingForm.cache_read, and pricingForm.cache_write only use number/step constraints. Add a non-negative input constraint to these fields in the pricing section of the [id].vue editor so the UI blocks accidental negative pricing; use the existing pricingForm bindings and keep the savePricingOverride path unchanged since it already handles validation.frontend/pages/settings/analytics.vue-8-9 (1)
8-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
user_idbefore passing it downstream.route.query.user_idcan be an array at runtime, and this page forwards it toSettingsAnalyticsDashboard/fetchUserAnalytics, which expect a single string. Converting repeated query params to one scalar avoids malformed analytics requests.Proposed fix
const route = useRoute(); -const userId = computed(() => route.query.user_id as string | undefined); +const userId = computed(() => { + const raw = route.query.user_id; + return Array.isArray(raw) ? (raw[0] ?? undefined) : (raw ?? undefined); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/settings/analytics.vue` around lines 8 - 9, Normalize the route query value in analytics.vue before it reaches SettingsAnalyticsDashboard and fetchUserAnalytics: `route.query.user_id` may be a string array at runtime, so update the `userId` computed value to always resolve to a single string or undefined. Use the existing `useRoute` and `userId` computed symbol to coerce repeated `user_id` params into one scalar so downstream analytics requests never receive an array.frontend/components/settings/AnalyticsDashboard.vue-541-556 (1)
541-556: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExplore tab exposes unimplemented group/metric options with no feedback.
exploreGroupOptionsincludesapi_keyandprovider(Lines 553-554), andexploreMetricOptionsincludeslatency(Line 548), butexploreAggregateRows/exploreChartData/exploreTableData(Lines 857-980) only implement'model'and'user'groups, andgetMetricValue/getSummaryMetricValuereturn0for'latency'. Selecting these renders a generic "no data" state instead of the "coming soon" treatment used elsewhere (e.g. Lines 122-132, 277-282), which can mislead users into thinking there's genuinely zero usage.Suggested fix
const exploreGroupOptions = computed(() => [ {value: 'model', label: tx('settings.analytics.model', 'Model')}, - {value: 'api_key', label: tx('settings.analytics.api_key', 'API key')}, - {value: 'provider', label: tx('settings.analytics.provider', 'Provider')}, {value: 'user', label: tx('settings.analytics.user', 'User')}, ]);Or, if these are intentionally forward-looking, disable the options in the select and mark them "coming soon" instead of leaving them selectable.
Also applies to: 821-980
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/settings/AnalyticsDashboard.vue` around lines 541 - 556, The Explore tab options are exposing unimplemented choices, so update AnalyticsDashboard.vue to avoid letting users select unsupported values. In exploreMetricOptions and exploreGroupOptions, either remove or disable the latency, api_key, and provider entries, or mark them as coming soon consistent with the existing coming-soon UI pattern used elsewhere in the component. Make sure the exploreGroupOptions/exploreMetricOptions consumers, along with exploreAggregateRows, exploreChartData, exploreTableData, getMetricValue, and getSummaryMetricValue, do not fall back to a misleading no-data state for these unsupported selections.src/routes/admin/budgets.rs-100-122 (1)
100-122: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
assign_budgetdoesn't validate thatteam_id/user_idis present.
unassign_budget(Line 163) rejects requests where bothteam_idanduser_idareNone, butassign_budgethas no equivalent check before callingbudget.assign. An assignment with neither scope set is likely meaningless and should be rejected the same way for consistency.Proposed fix
if !user.has_permission(&state.db, ADMIN_BUDGETS_EDIT).await { return ErrorBuilder::new(ErrorCode::InsufficientPermissions).build(); } + if req.team_id.is_none() && req.user_id.is_none() { + return ErrorBuilder::new(ErrorCode::ValidationFailed).build(); + } let budget = match Budget::find_by_id(&state.db, &id).await {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin/budgets.rs` around lines 100 - 122, assign_budget currently allows a BudgetAssignmentRequest with neither team_id nor user_id set, unlike unassign_budget. Add the same validation in assign_budget before calling Budget::assign, using BudgetAssignmentRequest and the existing ErrorBuilder flow to return the appropriate bad-request style error when both targets are missing. Keep the check near the request handling in assign_budget so invalid assignments are rejected consistently before any database mutation.src/routes/admin/budgets.rs-31-50 (1)
31-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNo validation that
amountis positive.
create_budgetonly checksreq.nameis non-empty; a zero or negativeamountwould silently create a budget that either instantly exhausts (0) or never enforces (negative), with no request-side guard.update_budgethas the same gap for its optionalamountfield.Proposed fix
if req.name.trim().is_empty() { return ErrorBuilder::new(ErrorCode::ValidationFailed).build(); } + if req.amount <= rust_decimal::Decimal::ZERO { + return ErrorBuilder::new(ErrorCode::ValidationFailed).build(); + } match Budget::create(&state.db, &req).await {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin/budgets.rs` around lines 31 - 50, Add request-side validation for budget amounts in create_budget and update_budget so zero or negative values are rejected before calling Budget::create or applying updates. Check req.amount in create_budget and the optional amount field in update_budget, returning ValidationFailed when the value is not strictly positive. Keep the existing name validation and permission checks, and use the existing CreateBudgetRequest/UpdateBudgetRequest handling paths to locate the fix.src/types/models/repository.rs-517-519 (1)
517-519: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winsrc/types/models/repository.rs:517 — Parse the reasoning override as
Decimal, notf64.decimal_from_jsononly feedsoverride_pricing["reasoning"]; parsing the JSON text directly avoids rounding drift in custom reasoning rates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/models/repository.rs` around lines 517 - 519, The reasoning override parsing in decimal_from_json currently converts through f64, which can introduce rounding drift for custom reasoning rates. Update decimal_from_json in repository.rs so it parses the JSON value directly into Decimal instead of using Value::as_f64, and keep the helper wired to the override_pricing["reasoning"] path so the reasoning override is preserved exactly.src/types/budgets/requests.rs-13-22 (1)
13-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate enum-backed string fields before hitting the DB cast.
kind,interval,reset_strategy, andon_exceedare unchecked strings. An invalid value will only be caught by the Postgres enum cast ($4::budget_kind, etc.) inrepository.rs, which the admin route maps to a genericDatabaseErrorrather than aValidationFailedresponse.💡 Proposed validation helper
+const VALID_KINDS: &[&str] = &["per_user", "pooled"]; +const VALID_INTERVALS: &[&str] = &["daily", "weekly", "monthly"]; +const VALID_RESET_STRATEGIES: &[&str] = &["rolling", "anchored", "calendar"]; +const VALID_ON_EXCEED: &[&str] = &["block", "warn", "allow"];Call this from the admin route handlers (or a shared validator) before invoking
Budget::create/update.Also applies to: 25-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/budgets/requests.rs` around lines 13 - 22, Validate the enum-backed string fields on CreateBudgetRequest before calling Budget::create or Budget::update, since kind, interval, reset_strategy, and on_exceed are currently unchecked and only fail later in repository.rs during Postgres enum casting. Add a shared validator for these fields (and reuse it in the admin route handlers for create/update) so invalid values return ValidationFailed instead of a generic DatabaseError; use the CreateBudgetRequest type and the budget repository methods as the key places to wire this in.
🧹 Nitpick comments (7)
frontend/components/settings/StackedAnalyticsChart.vue (1)
140-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTooltip-positioning logic duplicated with
HorizontalAnalyticsChart.vue.The
getBoundingClientRect()-based clamped tooltip positioning (Lines 241-255) closely mirrorssetHoveredRowinHorizontalAnalyticsChart.vue. Consider extracting a shared composable (e.g.useChartTooltipPosition) for both chart components.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/settings/StackedAnalyticsChart.vue` around lines 140 - 255, The tooltip positioning logic in setHoveredBar is duplicated from the horizontal chart’s setHoveredRow and should be shared. Extract the getBoundingClientRect-based clamping and coordinate calculation into a reusable composable such as useChartTooltipPosition, then have both StackedAnalyticsChart.vue and HorizontalAnalyticsChart.vue call it so tooltip placement stays consistent in one place.frontend/components/settings/AnalyticsDashboard.vue (2)
821-845: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate metric-value switch statements.
getMetricValueandgetSummaryMetricValueare identical switch statements operating on row types that share the same field names (cost_total,request_count,input_tokens,output_tokens,reasoning_tokens). Consider consolidating into one generic function.Suggested consolidation
-function getMetricValue(row: AnalyticsDayModelRow): number { - switch (exploreMetric.value) { - case 'cost': return Number(row.cost_total); - case 'requests': return row.request_count; - case 'tokens_total': return row.input_tokens + row.output_tokens + row.reasoning_tokens; - case 'tokens_input': return row.input_tokens; - case 'tokens_output': return row.output_tokens; - case 'tokens_reasoning': return row.reasoning_tokens; - case 'latency': return 0; - default: return Number(row.cost_total); - } -} - -function getSummaryMetricValue(row: AnalyticsRow): number { - switch (exploreMetric.value) { - case 'cost': return Number(row.cost_total); - case 'requests': return row.request_count; - case 'tokens_total': return row.input_tokens + row.output_tokens + row.reasoning_tokens; - case 'tokens_input': return row.input_tokens; - case 'tokens_output': return row.output_tokens; - case 'tokens_reasoning': return row.reasoning_tokens; - case 'latency': return 0; - default: return Number(row.cost_total); - } -} +type MetricSource = {cost_total: string; request_count: number; input_tokens: number; output_tokens: number; reasoning_tokens: number}; +function getMetricValue(row: MetricSource): number { + switch (exploreMetric.value) { + case 'cost': return Number(row.cost_total); + case 'requests': return row.request_count; + case 'tokens_total': return row.input_tokens + row.output_tokens + row.reasoning_tokens; + case 'tokens_input': return row.input_tokens; + case 'tokens_output': return row.output_tokens; + case 'tokens_reasoning': return row.reasoning_tokens; + case 'latency': return 0; + default: return Number(row.cost_total); + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/settings/AnalyticsDashboard.vue` around lines 821 - 845, The metric-value logic is duplicated in getMetricValue and getSummaryMetricValue, even though both row types expose the same fields. Consolidate the shared switch into a single reusable helper in AnalyticsDashboard.vue that accepts the row object and uses exploreMetric.value, then have both getMetricValue and getSummaryMetricValue delegate to it so the mapping is defined in one place.
765-793: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTrending-users sparkline is synthetic, not real historical data.
rankSparklineValuesfabricates a curve from the current value and the row's rank index rather than actual per-day spend (unliketrendingModels, which derives its sparkline from realseriesByModelday buckets, Line 760). This can visually imply a genuine trend that doesn't exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/settings/AnalyticsDashboard.vue` around lines 765 - 793, The trendingUsers sparkline is currently fabricated by rankSparklineValues using the current cost and index, which makes it look like real history when it is not. Update AnalyticsDashboard.vue so trendingUsers builds sparkline data from actual per-day user spend series, similar to how trendingModels uses real day buckets from seriesByModel, and remove or replace rankSparklineValues with logic that reads the underlying historical data instead of synthesizing values.src/routes/admin/budgets.rs (1)
15-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated auth+permission boilerplate across every handler.
The
get_current_user+has_permissionpair is duplicated verbatim in all 13 handlers in this file. Extracting a small helper (e.g.async fn require_permission(state, cookies, perm) -> Result<User, Response>) would cut significant duplication and reduce risk of a future handler forgetting the check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin/budgets.rs` around lines 15 - 241, The auth and permission checks are duplicated across all budget handlers, so extract the repeated `get_current_user` plus `has_permission` flow into a small reusable helper in this module, such as a `require_permission` function that returns the authenticated user or an error response. Update each handler like `list_budgets`, `create_budget`, `update_budget`, `delete_budget`, `assign_budget`, `get_budget_assignments`, `delete_assignment`, `unassign_budget`, `user_overview`, `team_overview`, `reset_history`, and `reset_budget` to call that helper instead of inlining the same boilerplate.src/routes/admin/analytics.rs (1)
12-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour near-identical dispatch branches for
group_by.The
user_id-present vs absent,day_modelvs other-group_by branches duplicate the same match/response/error pattern four times. Consider consolidating into a single async call whose target function is selected up front, e.g.:let result = match (params.user_id.as_ref(), group_by) { (Some(uid), "day_model") => UsageEvent::day_model_analytics(&state.db, params.from, params.to, Some(uid)).await.map(|r| /* wrap */), (Some(uid), _) => UsageEvent::analytics_for_user(&state.db, uid, params.from, params.to, group_by).await.map(|r| /* wrap */), (None, "day_model") => UsageEvent::day_model_analytics(&state.db, params.from, params.to, None).await.map(|r| /* wrap */), (None, _) => UsageEvent::analytics(&state.db, params.from, params.to, group_by).await.map(|r| /* wrap */), };Since the two analytics row types differ (
AnalyticsRowvsAnalyticsDayModelRow), full unification may need an enum wrapper, but even reducing the error-handling duplication (singleeprintln!/ErrorBuildersite) would help.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin/analytics.rs` around lines 12 - 54, The get_analytics handler has four near-identical branches for user_id presence and group_by, duplicating the same match, logging, and ErrorBuilder response logic. Refactor the branching around get_analytics and the UsageEvent::day_model_analytics, UsageEvent::analytics_for_user, and UsageEvent::analytics calls so the target query is chosen up front and the error/response handling is centralized in one place. If the return row types prevent full unification, use a small enum or shared wrapper to keep the dispatch logic separate from the repeated ResponseBuilder/ErrorBuilder boilerplate.src/types/budgets/mod.rs (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider modeling budget enums as Rust enums instead of raw
String.
kind,interval,reset_strategy, andon_exceedmirror Postgres enum types but are stored asString. Enforcement-critical comparisons inrepository.rs(e.g.budget.budget.on_exceed == "block",kind == "pooled") rely on string literals scattered across the file with no compiler-checked exhaustiveness. A typo or drift between the DB enum and a comparison string would silently break enforcement.Introducing small Rust enums (with
FromStr/Displaymapping to the DB text representation) would catch such mismatches at compile time and make the enforcement logic self-documenting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/budgets/mod.rs` around lines 16 - 28, The Budget model currently uses raw String fields for enum-backed values, which leaves enforcement logic dependent on scattered string literals. Update Budget to use Rust enums for kind, interval, reset_strategy, and on_exceed, and add FromStr/Display conversions to map to the existing Postgres text values. Then update repository.rs comparisons that currently check literals like "block" and "pooled" to use the new typed enums so the compiler can catch mismatches.src/types/budgets/repository.rs (1)
377-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated, intricate reset-matching logic between
latest_resets_for_userandteam_window.Both functions implement the same multi-branch OR matching (assignment-scoped reset / budget-wide reset / user-wide-by-kind reset / team-wide-by-kind reset) independently. This is easy to get subtly out of sync if the reset semantics ever change in one place but not the other, and is hard to reason about without inline documentation.
Consider extracting the shared predicate into a single SQL fragment/helper (or a documented constant string) reused by both call sites, and adding a short comment explaining each OR branch's intent.
Also applies to: 664-683
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/budgets/repository.rs` around lines 377 - 412, The reset-matching SQL is duplicated between latest_resets_for_user and team_window, so extract the shared multi-branch OR predicate into one reusable SQL fragment/helper or documented constant and have both queries use it. Keep the matching logic for assignment_id, budget_id, user_id, and team_id in one place, and add a brief inline comment explaining each branch’s intent so the semantics stay aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/components/chat/ModelSelector.vue`:
- Around line 313-323: The availability check in ModelSelector’s
default-provider logic is treating request failures the same as “not available,”
which causes default_provider_slug to be cleared on transient errors. Update the
try/catch flow around the $customFetch call so only a confirmed negative result
from the /provider-options response clears update.default_provider_slug, and
keep the existing value when the fetch fails; use the available flag and the
surrounding default model handling block to locate the change.
In `@frontend/components/settings/AnalyticsDashboard.vue`:
- Around line 1010-1027: The load() function in AnalyticsDashboard.vue only uses
try/finally, so rejected calls from fetchUserAnalytics, fetchAllAnalytics, or
fetchMyAnalytics are left unhandled and the UI never shows an error state.
Update load() to catch fetch failures, clear or set an explicit error state for
the dashboard, and surface the failure to the user while still keeping the
existing loading.value cleanup in finally. Use the load() method and the
budgetStore fetch* analytics methods as the key points to update.
In `@frontend/components/settings/HorizontalAnalyticsChart.vue`:
- Around line 43-54: The `@focus` handler on the SVG `rect` in
`HorizontalAnalyticsChart.vue` is unreachable because the element is not
keyboard-focusable by default. Update the interactive row element in the chart
rendering block so keyboard users can focus it, and ensure the existing
`setHoveredRow` behavior is triggered from both mouse and keyboard interaction.
If the tooltip should remain accessible, add the appropriate focusability and
accessibility attributes alongside the `rect` used for each row.
In `@frontend/components/settings/StackedAnalyticsChart.vue`:
- Around line 46-54: The bar hit-target in StackedAnalyticsChart.vue only
supports mouse hover, so keyboard users cannot access the per-bar tooltip data.
Update the rect element used in the chart interaction to be focusable and add
keyboard-friendly handlers (such as focus/blur and an Enter/Space-triggered
equivalent) that call the same setHoveredBar logic as `@mousemove`, so each bar
can expose its tooltip data without a mouse.
In `@frontend/pages/settings/budgets.vue`:
- Around line 534-542: Wrap the budget mutation handlers in
budgets.vue—createBudget, saveBudget, deleteBudget, assignTeam, assignUser,
removeAssignment, and confirmReset—in try/catch so backend failures don’t fail
silently. Follow the pattern used in teams.vue and users.vue: keep the existing
budgetStore calls, catch any thrown error, and show a store.toast message on
failure while preserving the current success behavior. Use the handler names and
budgetStore action calls as the anchor points when updating the UI flow.
In `@frontend/pages/settings/models/`[id].vue:
- Around line 1051-1067: The savePricingOverride function currently sends
pricing values without validating them, so negative or non-finite numbers can
reach the API and corrupt cost calculations. Update savePricingOverride in the
model settings page to validate each parsed field before building
overridePricing, rejecting any negative, NaN, or infinite values and preventing
the PUT request when invalid input is present. Use the existing pricingForm
fields and the overridePricing payload assembly as the hook points for the
guard.
In `@migrations/20260706000000_budgets_and_analytics.sql`:
- Line 29: The budgets table definition currently allows negative values for the
amount field, which can break downstream status logic. Update the migration that
defines the budgets schema to add a non-negative CHECK constraint on the amount
column, and make sure it is applied alongside the existing definition in the
budgets_and_analytics migration so invalid rows cannot be inserted.
- Around line 65-68: The budget/analytics schema allows negative usage and cost
values, so add boundary enforcement in the migration for the usage columns in
this table. Update the table definition in the budgets_and_analytics migration
so the token counters and cost_total are constrained to non-negative values,
using the table’s column definitions and any related insert/update path if
needed, with the relevant columns being input_tokens, output_tokens,
reasoning_tokens, and cost_total.
- Line 78: The model_pricing_overrides.pricing JSONB column is still
unconstrained even though ModelPricing::effective and
ModelPricing::priced_model_ids cast pricing->>'input' and pricing->>'output' to
numeric, so add validation in the migration to ensure those keys exist and
contain numeric-compatible values, or replace the JSONB shape with typed numeric
columns. Update the budgets_and_analytics migration where pricing is defined so
bad rows cannot break the queries that rely on ModelPricing and
ModelPricing::effective.
In `@migrations/20260706010000_budget_manual_resets.sql`:
- Around line 3-17: The budget_reset_events scope constraint is too loose and
conflicts with the current foreign-key delete behavior. Update the migration so
the budget_reset_events table enforces exactly one non-null scope field in
budget_reset_events_scope_check, and align the REFERENCES behavior for
assignment_id, budget_id, team_id, and user_id with the intended lifecycle so
deleting the scoped record does not leave an invalid row. Review the table
definition in the migration and keep the scope logic consistent with the
repository’s scope matching in the reset lookup path.
In `@src/routes/admin/budgets.rs`:
- Around line 156-173: The unassign_budget handler currently passes a
BudgetAssignmentRequest with only team_id/user_id into Budget::unassign, which
can delete multiple matching assignments at once. Update the request and/or
unassign flow so the endpoint targets one specific budget assignment by a unique
assignment identifier, and have Budget::unassign use that identifier instead of
broad team_id/user_id matching. Keep the validation in unassign_budget aligned
with the new required field and adjust any call sites or request structs
accordingly.
In `@src/routes/mod.rs`:
- Around line 63-64: The admin overview endpoints are still using per-entity
repository loops that can devolve into N+1 behavior. Update the handlers behind
admin::budgets::user_overview and admin::budgets::team_overview to batch status,
team, spend, and window lookups in the underlying repository/query layer before
returning results, and reuse those batched results in the route handlers instead
of calling helper queries inside each loop.
In `@src/routes/public/streaming.rs`:
- Around line 907-915: The cost selection logic in the streaming path is
incorrectly preferring catalog-derived pricing over the routed provider’s actual
cost. Update the `final_cost_total` computation in the streaming handler to keep
`cost_details.total` as the default when present, and only fall back to
`ModelPricing::usage_cost` when provider cost is absent or an explicit admin
override requires catalog pricing. Use the existing `provider_cost_total`,
`cost_details.total`, and `ModelPricing::usage_cost` logic in the streaming
route to locate and adjust this fallback order.
- Around line 936-953: The usage accounting path in the streaming handler is
only logging failures from UsageEvent::record and then still committing the
budget lock, which allows completion without a recorded charge. Update the
streaming flow around UsageEvent::record and budget_lock.commit in
streaming::... so that accounting is treated as terminal: either record usage
transactionally while the lock is held or return an error to the client instead
of proceeding to Done when recording fails. Make sure the failure path prevents
the lock from being released as a successful completion.
In `@src/types/budgets/repository.rs`:
- Around line 541-569: The `user_overview` method is doing an N+1 query pattern
by calling `Self::status_for_user` and `Self::teams_for_user` for each user
inside the loop. Refactor `user_overview` to fetch statuses, budgets, blocked
models, and team memberships in batched queries keyed by user IDs, then assemble
the `UserBudgetOverviewResponse` rows in memory using the existing
`user_overview`, `status_for_user`, and `teams_for_user` data shapes.
- Around line 586-662: The `team_overview` method has an N+1 query issue because
`Self::team_window` and `Self::team_assignment_spent` are executed for every
`TeamBudgetRow` inside the loop. Refactor `team_overview` to batch these lookups
or fold them into the main query so each team/budget assignment is resolved with
minimal round trips, while keeping the existing `TeamBudgetOverviewResponse` and
`TeamBudgetAssignmentOverviewResponse` assembly logic intact.
- Around line 605-662: `team_overview` is aggregating `"per_user"` team budgets
incorrectly by comparing pooled team spend against a multiplied capacity, which
can hide exhausted members. Update the logic in `team_overview` to mirror
`spent_by_assignment`/`status_for_user` for `is_pooled_team`: compute spend per
member for `"per_user"` budgets, count `exhausted_users` from members whose own
spend reaches the per-user `amount`, and keep the overview totals consistent
with the exposed `BudgetResponse.amount`. Ensure `remaining` and
`exhausted_count` are derived from the same per-user semantics so the returned
`TeamBudgetOverviewResponse` matches actual enforcement.
- Around line 180-224: Serialize budget reassignment for the same team/user and
budget kind by adding the same advisory-lock pattern used elsewhere before the
delete/insert flow in assign_to_team and assign_to_user. The current transaction
only protects the row changes, not concurrent reassignment of the same
team/user, so obtain the lock at the start of each method before running the
DELETE FROM budget_assignments and INSERT statements, using the budget kind plus
team_id/user_id as the lock key so only one reassignment can run at a time.
In `@src/types/models/repository.rs`:
- Around line 628-653: Update the pricing checks in repository::is_free and
repository::priced_model_ids to include the reasoning price alongside input and
output. In is_free, make the free/override logic require reasoning to be zero
too, and in priced_model_ids adjust the SQL/HAVING logic so any nonzero
reasoning price marks a model as priced/blocked. Use the existing effective
pricing flow in Self::effective and the pricing override/catalog joins as the
places to extend.
In `@src/types/usage/repository.rs`:
- Around line 61-72: The model/day usage aggregation query in usage repository
currently uses an inner join on models, which excludes historical usage_events
rows when usage_events.model_id becomes null after model deletion. Update the
query in the repository method that builds these aggregates to use a LEFT JOIN
from usage_events to models and add a fallback label for missing models so
deleted-model records still contribute to totals. Make the same join/label
adjustment in the other affected aggregation query referenced by the review so
the analytics remain consistent across both paths.
---
Outside diff comments:
In `@frontend/stores/chatStore.ts`:
- Around line 759-773: The `case 'done'` block in `chatStore.ts` contains a
lexical declaration (`const budgetStore`) without its own scope, which violates
the switch-scope rule. Wrap the entire `case 'done'` body in braces in the
switch handling around the `done` branch, keeping the existing `Object.assign`,
`this.isStreaming`, `chatIndex`/`chat` updates, and `useBudgetStore()` logic
inside that scoped block.
In `@src/types/teams/repository.rs`:
- Around line 185-201: The update_budget method currently performs the teams row
update and Budget::replace_team_budget_from_legacy as separate operations, which
can leave the legacy column and budget_assignments out of sync if the second
step fails. Update teams::repository::update_budget to run both writes through
the same transaction, or refactor Budget::replace_team_budget_from_legacy to
accept a shared executor so both steps can be executed atomically. Use the
existing update_budget and replace_team_budget_from_legacy symbols to keep the
fix localized.
---
Minor comments:
In `@frontend/components/settings/AnalyticsDashboard.vue`:
- Around line 541-556: The Explore tab options are exposing unimplemented
choices, so update AnalyticsDashboard.vue to avoid letting users select
unsupported values. In exploreMetricOptions and exploreGroupOptions, either
remove or disable the latency, api_key, and provider entries, or mark them as
coming soon consistent with the existing coming-soon UI pattern used elsewhere
in the component. Make sure the exploreGroupOptions/exploreMetricOptions
consumers, along with exploreAggregateRows, exploreChartData, exploreTableData,
getMetricValue, and getSummaryMetricValue, do not fall back to a misleading
no-data state for these unsupported selections.
In `@frontend/pages/settings/analytics.vue`:
- Around line 8-9: Normalize the route query value in analytics.vue before it
reaches SettingsAnalyticsDashboard and fetchUserAnalytics: `route.query.user_id`
may be a string array at runtime, so update the `userId` computed value to
always resolve to a single string or undefined. Use the existing `useRoute` and
`userId` computed symbol to coerce repeated `user_id` params into one scalar so
downstream analytics requests never receive an array.
In `@frontend/pages/settings/models/`[id].vue:
- Around line 840-884: The pricing override form in the settings models editor
allows negative values because the ShadInput fields for pricingForm.input,
pricingForm.output, pricingForm.reasoning, pricingForm.cache_read, and
pricingForm.cache_write only use number/step constraints. Add a non-negative
input constraint to these fields in the pricing section of the [id].vue editor
so the UI blocks accidental negative pricing; use the existing pricingForm
bindings and keep the savePricingOverride path unchanged since it already
handles validation.
In `@src/routes/admin/budgets.rs`:
- Around line 100-122: assign_budget currently allows a BudgetAssignmentRequest
with neither team_id nor user_id set, unlike unassign_budget. Add the same
validation in assign_budget before calling Budget::assign, using
BudgetAssignmentRequest and the existing ErrorBuilder flow to return the
appropriate bad-request style error when both targets are missing. Keep the
check near the request handling in assign_budget so invalid assignments are
rejected consistently before any database mutation.
- Around line 31-50: Add request-side validation for budget amounts in
create_budget and update_budget so zero or negative values are rejected before
calling Budget::create or applying updates. Check req.amount in create_budget
and the optional amount field in update_budget, returning ValidationFailed when
the value is not strictly positive. Keep the existing name validation and
permission checks, and use the existing CreateBudgetRequest/UpdateBudgetRequest
handling paths to locate the fix.
In `@src/types/budgets/requests.rs`:
- Around line 13-22: Validate the enum-backed string fields on
CreateBudgetRequest before calling Budget::create or Budget::update, since kind,
interval, reset_strategy, and on_exceed are currently unchecked and only fail
later in repository.rs during Postgres enum casting. Add a shared validator for
these fields (and reuse it in the admin route handlers for create/update) so
invalid values return ValidationFailed instead of a generic DatabaseError; use
the CreateBudgetRequest type and the budget repository methods as the key places
to wire this in.
In `@src/types/models/repository.rs`:
- Around line 517-519: The reasoning override parsing in decimal_from_json
currently converts through f64, which can introduce rounding drift for custom
reasoning rates. Update decimal_from_json in repository.rs so it parses the JSON
value directly into Decimal instead of using Value::as_f64, and keep the helper
wired to the override_pricing["reasoning"] path so the reasoning override is
preserved exactly.
---
Nitpick comments:
In `@frontend/components/settings/AnalyticsDashboard.vue`:
- Around line 821-845: The metric-value logic is duplicated in getMetricValue
and getSummaryMetricValue, even though both row types expose the same fields.
Consolidate the shared switch into a single reusable helper in
AnalyticsDashboard.vue that accepts the row object and uses exploreMetric.value,
then have both getMetricValue and getSummaryMetricValue delegate to it so the
mapping is defined in one place.
- Around line 765-793: The trendingUsers sparkline is currently fabricated by
rankSparklineValues using the current cost and index, which makes it look like
real history when it is not. Update AnalyticsDashboard.vue so trendingUsers
builds sparkline data from actual per-day user spend series, similar to how
trendingModels uses real day buckets from seriesByModel, and remove or replace
rankSparklineValues with logic that reads the underlying historical data instead
of synthesizing values.
In `@frontend/components/settings/StackedAnalyticsChart.vue`:
- Around line 140-255: The tooltip positioning logic in setHoveredBar is
duplicated from the horizontal chart’s setHoveredRow and should be shared.
Extract the getBoundingClientRect-based clamping and coordinate calculation into
a reusable composable such as useChartTooltipPosition, then have both
StackedAnalyticsChart.vue and HorizontalAnalyticsChart.vue call it so tooltip
placement stays consistent in one place.
In `@src/routes/admin/analytics.rs`:
- Around line 12-54: The get_analytics handler has four near-identical branches
for user_id presence and group_by, duplicating the same match, logging, and
ErrorBuilder response logic. Refactor the branching around get_analytics and the
UsageEvent::day_model_analytics, UsageEvent::analytics_for_user, and
UsageEvent::analytics calls so the target query is chosen up front and the
error/response handling is centralized in one place. If the return row types
prevent full unification, use a small enum or shared wrapper to keep the
dispatch logic separate from the repeated ResponseBuilder/ErrorBuilder
boilerplate.
In `@src/routes/admin/budgets.rs`:
- Around line 15-241: The auth and permission checks are duplicated across all
budget handlers, so extract the repeated `get_current_user` plus
`has_permission` flow into a small reusable helper in this module, such as a
`require_permission` function that returns the authenticated user or an error
response. Update each handler like `list_budgets`, `create_budget`,
`update_budget`, `delete_budget`, `assign_budget`, `get_budget_assignments`,
`delete_assignment`, `unassign_budget`, `user_overview`, `team_overview`,
`reset_history`, and `reset_budget` to call that helper instead of inlining the
same boilerplate.
In `@src/types/budgets/mod.rs`:
- Around line 16-28: The Budget model currently uses raw String fields for
enum-backed values, which leaves enforcement logic dependent on scattered string
literals. Update Budget to use Rust enums for kind, interval, reset_strategy,
and on_exceed, and add FromStr/Display conversions to map to the existing
Postgres text values. Then update repository.rs comparisons that currently check
literals like "block" and "pooled" to use the new typed enums so the compiler
can catch mismatches.
In `@src/types/budgets/repository.rs`:
- Around line 377-412: The reset-matching SQL is duplicated between
latest_resets_for_user and team_window, so extract the shared multi-branch OR
predicate into one reusable SQL fragment/helper or documented constant and have
both queries use it. Keep the matching logic for assignment_id, budget_id,
user_id, and team_id in one place, and add a brief inline comment explaining
each branch’s intent so the semantics stay aligned.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2471bc0a-440b-4c4e-91e8-6ecfe48ff782
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
Cargo.tomlDockerfilefrontend/components/AppSidebar.vuefrontend/components/chat/ModelSelector.vuefrontend/components/settings/AnalyticsDashboard.vuefrontend/components/settings/AnalyticsDateFilter.vuefrontend/components/settings/HorizontalAnalyticsChart.vuefrontend/components/settings/StackedAnalyticsChart.vuefrontend/pages/settings.vuefrontend/pages/settings/analytics.vuefrontend/pages/settings/budgets.vuefrontend/pages/settings/models/[id].vuefrontend/pages/settings/profile.vuefrontend/pages/settings/teams.vuefrontend/pages/settings/users.vuefrontend/stores/budgetStore.tsfrontend/stores/chatStore.tsfrontend/types/budgets.tsfrontend/types/chat.tsmigrations/20260706000000_budgets_and_analytics.sqlmigrations/20260706010000_budget_manual_resets.sqlsrc/ai.rssrc/routes/admin/analytics.rssrc/routes/admin/budgets.rssrc/routes/admin/mod.rssrc/routes/admin/models.rssrc/routes/mod.rssrc/routes/public/models.rssrc/routes/public/streaming.rssrc/routes/public/users.rssrc/types/budgets/mod.rssrc/types/budgets/repository.rssrc/types/budgets/requests.rssrc/types/budgets/responses.rssrc/types/budgets/rows.rssrc/types/mod.rssrc/types/models/mod.rssrc/types/models/repository.rssrc/types/models/requests.rssrc/types/models/responses.rssrc/types/models/rows.rssrc/types/permissions/mod.rssrc/types/teams/repository.rssrc/types/usage/mod.rssrc/types/usage/repository.rssrc/utils/providers.rs
- ModelSelector: guard default_provider_slug clear behind checkedAvailability flag to avoid clearing on transient fetch failures - AnalyticsDashboard: add catch block to load() to surface analytics fetch errors via toast - HorizontalAnalyticsChart: add tabindex/blur to rect so keyboard users can access tooltip - StackedAnalyticsChart: add tabindex/focus/blur handlers to hit-target rect for keyboard accessibility - budgets.vue: wrap all budget CRUD and assignment mutations in try/catch with error toast - models/[id].vue: validate pricing fields are non-negative and finite before submitting override - Migration budgets_and_analytics: add CHECK (amount >= 0) on budgets.amount, CHECK constraints on usage_events token/cost columns, and CHECK constraint on model_pricing_overrides.pricing JSONB to require non-negative input/output keys - Migration budget_manual_resets: change ON DELETE SET NULL to ON DELETE CASCADE for scope columns, enforce exactly one non-null scope with num_nonnulls() - Remove legacy unassign_budget endpoint (delete by team/user_id) since delete_assignment endpoint by unique ID already exists - streaming.rs: prefer actual provider cost (cost_details.total) over catalog pricing; fail stream with error event when usage accounting fails - budgets/repository: add pg_advisory_xact_lock to assign_to_team/assign_to_user to prevent concurrent reassignment races - models/repository: include reasoning price in is_free and priced_model_ids checks - usage/repository: switch by_model and by_day_by_model to LEFT JOIN so usage rows with deleted models are preserved in analytics Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/public/streaming.rs (2)
937-968: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAccounting-failure path leaves an orphaned assistant message.
The failure handling now correctly terminates instead of silently completing (resolving the prior concern). However, the assistant message is already persisted at Lines 920-935 before
UsageEvent::record; on the failure branch (Lines 958-966) the lock rolls back and no usage row is written, but the saved message remains in the DB. The user is told accounting failed yet the response persists and re-appears on reload, with no spend recorded. Consider recording usage within the same transaction as message creation, or deleting/marking the message on accounting failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/public/streaming.rs` around lines 937 - 968, The accounting-failure branch in streaming::agentic_loop leaves a persisted assistant message behind because the message is saved before UsageEvent::record and the Err path only exits after rolling back the lock. Update the flow around the message persistence and UsageEvent::record calls so they are atomic, or explicitly delete/mark the saved assistant message when usage recording fails, using the existing msg handling and budget_lock commit/rollback paths as the anchor points.
433-461: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBudget lock keeps a pool connection open for the whole stream
acquire_budget_lockstarts a transaction and takespg_advisory_xact_lock, then that transaction is carried into the stream and only committed after usage is recorded. That pins one pool connection for the full upstream LLM stream, and concurrent same-user requests will also block while holding their own connections. Consider moving the lock to a short final transaction around the usage check/record step instead of holding it across streaming.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/public/streaming.rs` around lines 433 - 461, The budget locking in the streaming path is holding a database transaction open for the entire upstream response, which pins a pool connection and blocks concurrent same-user requests. Update the logic around acquire_budget_lock and the streaming flow in streaming.rs so the advisory lock is only used in a short-lived transaction around the final usage check/record step, not stored in budget_lock across the full stream; keep the rest of the streaming path free of the transaction/connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/20260706010000_budget_manual_resets.sql`:
- Around line 12-14: The budget reset validation only blocks the all-empty case,
so requests with multiple scope fields still reach Budget::reset and then fail
on budget_reset_events_scope_check at insert time. Update the admin budget reset
path in the route handler that calls Budget::reset to require exactly one of
assignment_id, budget_id, team_id, or user_id before proceeding, and return the
existing validation error early when the payload has zero or more than one scope
selected.
---
Outside diff comments:
In `@src/routes/public/streaming.rs`:
- Around line 937-968: The accounting-failure branch in streaming::agentic_loop
leaves a persisted assistant message behind because the message is saved before
UsageEvent::record and the Err path only exits after rolling back the lock.
Update the flow around the message persistence and UsageEvent::record calls so
they are atomic, or explicitly delete/mark the saved assistant message when
usage recording fails, using the existing msg handling and budget_lock
commit/rollback paths as the anchor points.
- Around line 433-461: The budget locking in the streaming path is holding a
database transaction open for the entire upstream response, which pins a pool
connection and blocks concurrent same-user requests. Update the logic around
acquire_budget_lock and the streaming flow in streaming.rs so the advisory lock
is only used in a short-lived transaction around the final usage check/record
step, not stored in budget_lock across the full stream; keep the rest of the
streaming path free of the transaction/connection.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b911b32a-e6e7-45b3-88ab-57d39bda45b4
📒 Files selected for processing (14)
frontend/components/chat/ModelSelector.vuefrontend/components/settings/AnalyticsDashboard.vuefrontend/components/settings/HorizontalAnalyticsChart.vuefrontend/components/settings/StackedAnalyticsChart.vuefrontend/pages/settings/budgets.vuefrontend/pages/settings/models/[id].vuemigrations/20260706000000_budgets_and_analytics.sqlmigrations/20260706010000_budget_manual_resets.sqlsrc/routes/admin/budgets.rssrc/routes/mod.rssrc/routes/public/streaming.rssrc/types/budgets/repository.rssrc/types/models/repository.rssrc/types/usage/repository.rs
💤 Files with no reviewable changes (2)
- src/routes/mod.rs
- src/routes/admin/budgets.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- frontend/components/chat/ModelSelector.vue
- frontend/pages/settings/models/[id].vue
- frontend/components/settings/HorizontalAnalyticsChart.vue
- frontend/components/settings/AnalyticsDashboard.vue
- frontend/components/settings/StackedAnalyticsChart.vue
- src/types/usage/repository.rs
- migrations/20260706000000_budgets_and_analytics.sql
- src/types/models/repository.rs
- frontend/pages/settings/budgets.vue
- src/types/budgets/repository.rs
Comment on lines
+12
to
+14
| CONSTRAINT budget_reset_events_scope_check CHECK ( | ||
| num_nonnulls(assignment_id, budget_id, team_id, user_id) = 1 | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect BudgetResetRequest construction/validation and the reset() bind path
rg -nP -C4 'struct\s+BudgetResetRequest' src/types/budgets
ast-grep run --pattern 'pub async fn reset($$$) { $$$ }' --lang rust src/types/budgets/repository.rs
# Look for any validation ensuring a single scope before reset
rg -nP -C3 '(assignment_id|budget_id|team_id|user_id)' src/routes/admin/budgets.rsRepository: NxtCore/OxideChat
Length of output: 1834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- requests.rs outline ---'
ast-grep outline src/types/budgets/requests.rs --view expanded || true
echo '--- repository.rs outline ---'
ast-grep outline src/types/budgets/repository.rs --view expanded || true
echo '--- relevant slices ---'
nl -ba src/types/budgets/requests.rs | sed -n '35,80p'
echo '---'
nl -ba src/types/budgets/repository.rs | sed -n '1,220p'
echo '--- budgets route slice ---'
nl -ba src/routes/admin/budgets.rs | sed -n '200,230p'Repository: NxtCore/OxideChat
Length of output: 5549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/types/budgets/repository.rs (reset body) ---'
sed -n '721,760p' src/types/budgets/repository.rs
echo '--- src/routes/admin/budgets.rs (reset validation) ---'
sed -n '205,220p' src/routes/admin/budgets.rs
echo '--- search for any single-scope validation ---'
rg -n -C 2 'BudgetResetRequest|num_nonnulls|exactly one|ValidationFailed|assignment_id.is_none|budget_id.is_none|team_id.is_none|user_id.is_none' src/types/budgets src/routes/admin/budgets.rsRepository: NxtCore/OxideChat
Length of output: 6548
Require exactly one reset scope before insert
src/routes/admin/budgets.rs:212-216 only rejects the all-none case. A payload with 2+ of assignment_id, budget_id, team_id, or user_id still reaches Budget::reset(...), where the insert now violates budget_reset_events_scope_check and fails at runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migrations/20260706010000_budget_manual_resets.sql` around lines 12 - 14, The
budget reset validation only blocks the all-empty case, so requests with
multiple scope fields still reach Budget::reset and then fail on
budget_reset_events_scope_check at insert time. Update the admin budget reset
path in the route handler that calls Budget::reset to require exactly one of
assignment_id, budget_id, team_id, or user_id before proceeding, and return the
existing validation error early when the payload has zero or more than one scope
selected.
… budget logic - user_overview: batch-fetch all effective budgets and team memberships for all users in two queries; reuse priced_model_ids across users; eliminates O(N*3)+O(N) queries down to ~5 total queries - team_overview: replace per-row team_window and team_assignment_spent calls with batch_team_window_resets and batch_team_member_spent (one UNNEST query each); eliminates O(2N) round trips - Fix per_user kind logic in team_overview: compute per-member spend individually, count exhausted_users as members whose spend >= per-user amount, keep spent+remaining consistent with unexpanded budget.amount Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add server-side validation for budget amount and enum variants (kind, interval, reset_strategy, on_exceed) - Enforce presence of either `team_id` or `user_id` in budget assignments - Improve frontend toast notifications for budget CRUD and assignment actions - Add `min="0"` constraints to pricing override inputs in model settings - Refactor analytics dashboard to use shared metric value logic and simplify grouped options - Fix `.gitignore` typo in `.codegraph` directory
…t auto-review - Add i18n keys for budget CRUD, assignment, and reset actions in English and German - Update `.coderabbit.yaml` to disable `auto_review`
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
1 participant
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.
Greptile Summary
This PR introduces a full budgets and analytics system: per-user/team spending limits with configurable intervals, reset strategies, and enforcement modes (block/warn/allow), plus admin and per-user analytics dashboards backed by a new
usage_eventstable. The TOCTOU budget bypass from a previous review has been addressed with apg_advisory_xact_lockthat serializes per-user streaming requests.UsageEvent::recordcompletes — concurrent requests from the same user block at lock acquisition and see the updated spend on their own check.user_overview,team_overview): both load all records in a single batch query but then call per-row DB helpers inside loops, producing O(4N) and O(2M) round-trips respectively — AGENTS.md explicitly prohibits this pattern.Confidence Score: 4/5
Safe to merge for correctness and security; the admin budget overview pages will degrade in performance as user/team counts grow.
The streaming budget lock correctly serializes per-user requests and the hot-path spend queries are properly batched. The remaining concern is that both admin overview endpoints iterate over all users and team-budget assignments and fire multiple DB queries per row, which is a direct violation of the explicit no-N+1 rule in AGENTS.md and will cause noticeable slowdowns on any deployment with more than a few dozen users or team assignments.
src/types/budgets/repository.rs — specifically the
user_overviewandteam_overviewmethods.Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant StreamHandler participant DB as PostgreSQL Client->>StreamHandler: POST /stream (model_key, content) StreamHandler->>DB: ModelPricing::is_free(model_id) DB-->>StreamHandler: false (priced model) StreamHandler->>DB: pg_advisory_xact_lock(hash(user_id)) [BEGIN TX] Note over StreamHandler,DB: Lock held — concurrent requests block here StreamHandler->>DB: Budget::status_for_user(user_id) DB-->>StreamHandler: "UserBudgetStatus { decision, blocked_model_ids }" alt model blocked StreamHandler-->>Client: SSE error: budget_exceeded Note over DB: TX dropped → lock released else model allowed StreamHandler->>Client: SSE stream (tokens) StreamHandler->>DB: UsageEvent::record(pool) [separate connection] StreamHandler->>DB: COMMIT TX → lock released Note over DB: Next queued request can now acquire lock end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Client participant StreamHandler participant DB as PostgreSQL Client->>StreamHandler: POST /stream (model_key, content) StreamHandler->>DB: ModelPricing::is_free(model_id) DB-->>StreamHandler: false (priced model) StreamHandler->>DB: pg_advisory_xact_lock(hash(user_id)) [BEGIN TX] Note over StreamHandler,DB: Lock held — concurrent requests block here StreamHandler->>DB: Budget::status_for_user(user_id) DB-->>StreamHandler: "UserBudgetStatus { decision, blocked_model_ids }" alt model blocked StreamHandler-->>Client: SSE error: budget_exceeded Note over DB: TX dropped → lock released else model allowed StreamHandler->>Client: SSE stream (tokens) StreamHandler->>DB: UsageEvent::record(pool) [separate connection] StreamHandler->>DB: COMMIT TX → lock released Note over DB: Next queued request can now acquire lock endComments Outside Diff (1)
src/routes/public/streaming.rs, line 387 (link)eprint!is missing the trailing newline that all other log statements in this file use, so this log line will be fused onto the same terminal line as whatever is printed next.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (2): Last reviewed commit: "Add budget reset functionality, enhanced..." | Re-trigger Greptile
Context used:
Summary by CodeRabbit