fix(platform): enforce budget max request limits for all period types#1464
Merged
Conversation
…#1452) Budget rules with daily/weekly periods were never enforced because usage was only recorded under the monthly period key. Daily/weekly budget checks found zero usage and always passed. - Record usage for all three periods (daily, weekly, monthly) in a single atomic mutation instead of only the default monthly period - Fix stale-reference bug in the dedup merge loop that could undercount usage when 3+ duplicate ledger rows existed - Use the thread's team context (user-selected team) for usage attribution instead of the agent's configured team list, fixing N-times inflation for multi-team agents - Add tests for daily/weekly budget rule enforcement and mixed-period rule collection
… pages Extract budget banner into standalone BudgetBanner component rendered outside the scroll container so it stays fixed at the top. Banner is now visible on thread pages (not just new chat) and always dismissible.
📝 WalkthroughWalkthroughThis pull request implements multi-period budget enforcement for governance rules. It extracts budget warning logic from the chat interface into a standalone Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform/convex/governance/internal_mutations.ts (1)
15-17: 🧹 Nitpick | 🔵 TrivialConsider removing the unused
periodargument.The
args.periodparameter is declared but never used since the mutation now iterates over all periods (ALL_PERIODS). This could confuse future maintainers.♻️ Suggested cleanup
args: { organizationId: v.string(), userId: v.string(), teamId: v.optional(v.string()), inputTokens: v.number(), outputTokens: v.number(), costEstimateCents: v.number(), timestamp: v.number(), - period: v.optional( - v.union(v.literal('daily'), v.literal('weekly'), v.literal('monthly')), - ), },Also applies to: 21-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/governance/internal_mutations.ts` around lines 15 - 17, The declared v.optional period argument (args.period) is unused because the mutation iterates over ALL_PERIODS; remove the unused period schema entry and any references to args.period to avoid confusion: update the mutation input schema by deleting the period field (the v.optional(v.union(...)) block) and ensure the handler logic (where ALL_PERIODS is used) does not expect or reference args.period so code and types remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/features/chat/components/budget-banner.tsx`:
- Line 63: The code is falling back to a hardcoded 'monthly' token and directly
interpolating raw period tokens (budgetStatus.period / w.period) into the UI;
update the BudgetBanner component to map period values to localized labels using
the translation hook (e.g., call t or useLocaleLabel) before interpolation
instead of using 'monthly' literal or raw tokens, replace occurrences where
period is passed (budgetStatus.period ?? 'monthly' and w.period) with a
mappedTranslatedPeriod variable created via a switch/lookup that calls the
translation function for 'monthly', 'weekly', 'yearly', etc., and use that
mappedTranslatedPeriod in the JSX so no untranslated/internal tokens are
rendered (also apply same change at the other occurrence flagged).
- Around line 24-31: Update the banner div that renders the budget status (the
element using cn(...) and budgetStatus.exceeded) to include live-region
semantics: add aria-atomic="true" and set aria-live dynamically (use
aria-live="assertive" when budgetStatus.exceeded is true and aria-live="polite"
otherwise); also set role="alert" for the exceeded case and role="status" for
non-critical updates so screen readers announce changes appropriately.
- Around line 95-102: The close button lacks an accessible focus outline and a
minimum 24×24px touch target; update the button in budget-banner.tsx (the button
that calls setDismissed(true) and renders <X />) to include an explicit
focus-visible ring and ensure at least 24×24 CSS pixels hit area (e.g., add
focus-visible classes like focus-visible:ring-2 focus-visible:ring-offset-1 (or
your design token equivalents), rounded corners, and minimum sizing/padding such
as min-w-6 min-h-6 or w-6 h-6 with p-1) so it is keyboard-focus visible and
meets the minimum touch target while keeping the existing aria-label and click
handler.
- Line 50: Replace uses of value?.toLocaleString() with an explicit
Intl.NumberFormat instance: create a single formatter (e.g., const
numberFormatter = new Intl.NumberFormat('en-US')) near the top of the
BudgetBanner component and call numberFormatter.format(value) for the four
occurrences (references to budgetStatus.used, budgetStatus.remaining, and the
two other numeric displays in the component). Guard null/undefined the same way
you did before (e.g., value == null ? '' : numberFormatter.format(value)) so
presentation doesn't change.
---
Outside diff comments:
In `@services/platform/convex/governance/internal_mutations.ts`:
- Around line 15-17: The declared v.optional period argument (args.period) is
unused because the mutation iterates over ALL_PERIODS; remove the unused period
schema entry and any references to args.period to avoid confusion: update the
mutation input schema by deleting the period field (the v.optional(v.union(...))
block) and ensure the handler logic (where ALL_PERIODS is used) does not expect
or reference args.period so code and types remain consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b90b27f-a77b-441b-86ac-54ff74b93d9a
📒 Files selected for processing (8)
services/platform/app/features/chat/components/budget-banner.tsxservices/platform/app/features/chat/components/chat-interface.tsxservices/platform/app/routes/dashboard/$id/chat.tsxservices/platform/convex/governance/__tests__/budget_enforcement.test.tsservices/platform/convex/governance/internal_mutations.tsservices/platform/convex/lib/agent_chat/internal_actions.tsservices/platform/convex/lib/agent_chat/start_agent_chat.tsservices/platform/convex/lib/agent_completion/on_agent_complete.ts
Comment on lines
+24
to
+31
| <div | ||
| className={cn( | ||
| 'flex items-center gap-2 border-b px-4 py-2', | ||
| budgetStatus.exceeded | ||
| ? 'bg-destructive/10 border-destructive/30' | ||
| : 'bg-warning/10 border-warning/30', | ||
| )} | ||
| > |
There was a problem hiding this comment.
Announce budget changes via a live region.
This banner is a dynamic status/alert update; add live-region semantics so screen readers announce changes consistently.
Suggested patch
<div
+ role={budgetStatus.exceeded ? 'alert' : 'status'}
+ aria-live={budgetStatus.exceeded ? 'assertive' : 'polite'}
+ aria-atomic="true"
className={cn(
'flex items-center gap-2 border-b px-4 py-2',
budgetStatus.exceededAs per coding guidelines: “USE aria-live="polite" for non-urgent updates … USE aria-live="assertive" only for critical alerts … USE aria-atomic="true" when the entire region should be re-read.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div | |
| className={cn( | |
| 'flex items-center gap-2 border-b px-4 py-2', | |
| budgetStatus.exceeded | |
| ? 'bg-destructive/10 border-destructive/30' | |
| : 'bg-warning/10 border-warning/30', | |
| )} | |
| > | |
| <div | |
| role={budgetStatus.exceeded ? 'alert' : 'status'} | |
| aria-live={budgetStatus.exceeded ? 'assertive' : 'polite'} | |
| aria-atomic="true" | |
| className={cn( | |
| 'flex items-center gap-2 border-b px-4 py-2', | |
| budgetStatus.exceeded | |
| ? 'bg-destructive/10 border-destructive/30' | |
| : 'bg-warning/10 border-warning/30', | |
| )} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/budget-banner.tsx` around
lines 24 - 31, Update the banner div that renders the budget status (the element
using cn(...) and budgetStatus.exceeded) to include live-region semantics: add
aria-atomic="true" and set aria-live dynamically (use aria-live="assertive" when
budgetStatus.exceeded is true and aria-live="polite" otherwise); also set
role="alert" for the exceeded case and role="status" for non-critical updates so
screen readers announce changes appropriately.
| const used = | ||
| isCost && budgetStatus.used != null | ||
| ? `$${(budgetStatus.used / 100).toFixed(2)}` | ||
| : budgetStatus.used?.toLocaleString(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and get its size
wc -l services/platform/app/features/chat/components/budget-banner.tsxRepository: tale-project/tale
Length of output: 129
🏁 Script executed:
# Read the entire file to verify the violations
cat -n services/platform/app/features/chat/components/budget-banner.tsxRepository: tale-project/tale
Length of output: 4377
Replace toLocaleString() with explicit number formatting.
toLocaleString() is disallowed in this repo; use Intl.NumberFormat instead for all number formatting on lines 50, 54, 80, and 84.
Suggested patch
export function BudgetBanner({ organizationId }: { organizationId: string }) {
const { t } = useT('chat');
const { data: budgetStatus } = useMyBudgetStatus(organizationId);
const [dismissed, setDismissed] = useState(false);
+ const numberFormatter = new Intl.NumberFormat();
@@
const used =
isCost && budgetStatus.used != null
? `$${(budgetStatus.used / 100).toFixed(2)}`
- : budgetStatus.used?.toLocaleString();
+ : budgetStatus.used != null
+ ? numberFormatter.format(budgetStatus.used)
+ : undefined;
const limit =
isCost && budgetStatus.limit != null
? `$${(budgetStatus.limit / 100).toFixed(2)}`
- : budgetStatus.limit?.toLocaleString();
+ : budgetStatus.limit != null
+ ? numberFormatter.format(budgetStatus.limit)
+ : undefined;
@@
const used =
w.code === 'COST_WARNING'
? `$${(w.used / 100).toFixed(2)}`
- : w.used.toLocaleString();
+ : numberFormatter.format(w.used);
const limit =
w.code === 'COST_WARNING'
? `$${(w.limit / 100).toFixed(2)}`
- : w.limit.toLocaleString();
+ : numberFormatter.format(w.limit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/budget-banner.tsx` at line 50,
Replace uses of value?.toLocaleString() with an explicit Intl.NumberFormat
instance: create a single formatter (e.g., const numberFormatter = new
Intl.NumberFormat('en-US')) near the top of the BudgetBanner component and call
numberFormatter.format(value) for the four occurrences (references to
budgetStatus.used, budgetStatus.remaining, and the two other numeric displays in
the component). Guard null/undefined the same way you did before (e.g., value ==
null ? '' : numberFormatter.format(value)) so presentation doesn't change.
| return used != null && limit != null | ||
| ? t('budgetExceededDetail', { | ||
| type, | ||
| period: budgetStatus.period ?? 'monthly', |
There was a problem hiding this comment.
Avoid untranslated period tokens in user-facing text.
'monthly' fallback and raw w.period values can leak English/internal tokens in localized UI; map period values to translated labels before interpolation.
As per coding guidelines: “Do NOT hardcode text, use the translation hooks/functions instead for user-facing UI.”
Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/budget-banner.tsx` at line 63,
The code is falling back to a hardcoded 'monthly' token and directly
interpolating raw period tokens (budgetStatus.period / w.period) into the UI;
update the BudgetBanner component to map period values to localized labels using
the translation hook (e.g., call t or useLocaleLabel) before interpolation
instead of using 'monthly' literal or raw tokens, replace occurrences where
period is passed (budgetStatus.period ?? 'monthly' and w.period) with a
mappedTranslatedPeriod variable created via a switch/lookup that calls the
translation function for 'monthly', 'weekly', 'yearly', etc., and use that
mappedTranslatedPeriod in the JSX so no untranslated/internal tokens are
rendered (also apply same change at the other occurrence flagged).
Comment on lines
+95
to
+102
| <button | ||
| type="button" | ||
| onClick={() => setDismissed(true)} | ||
| className="text-muted-foreground hover:text-foreground shrink-0" | ||
| aria-label={t('budgetWarningDismiss')} | ||
| > | ||
| <X className="size-4" /> | ||
| </button> |
There was a problem hiding this comment.
Dismiss button needs visible focus and minimum hit area.
The close control should meet keyboard-focus visibility and minimum touch-target size requirements.
Suggested patch
<button
type="button"
onClick={() => setDismissed(true)}
- className="text-muted-foreground hover:text-foreground shrink-0"
+ className="text-muted-foreground hover:text-foreground focus-visible:ring-ring shrink-0 inline-flex size-6 items-center justify-center rounded focus-visible:ring-2 focus-visible:outline-none"
aria-label={t('budgetWarningDismiss')}
>
- <X className="size-4" />
+ <X className="size-4" aria-hidden="true" />
</button>As per coding guidelines: “ENSURE all interactive elements are keyboard accessible and have visible focus states” and “ENSURE interactive elements have a minimum 24×24 CSS pixel touch target.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/budget-banner.tsx` around
lines 95 - 102, The close button lacks an accessible focus outline and a minimum
24×24px touch target; update the button in budget-banner.tsx (the button that
calls setDismissed(true) and renders <X />) to include an explicit focus-visible
ring and ensure at least 24×24 CSS pixels hit area (e.g., add focus-visible
classes like focus-visible:ring-2 focus-visible:ring-offset-1 (or your design
token equivalents), rounded corners, and minimum sizing/padding such as min-w-6
min-h-6 or w-6 h-6 with p-1) so it is keyboard-focus visible and meets the
minimum touch target while keeping the existing aria-label and click handler.
The budget banner useEffect depended on the budgetStatus object reference, which changes on every Convex subscription tick even when data is unchanged, making the banner impossible to dismiss. Replace with a derived stable key that only resets dismissed state on meaningful status changes. Also change `return null` to `continue` in the incrementUsageLedger dedup guard so that an unreachable defensive check cannot accidentally skip remaining period writes.
Usage ledger entries are now keyed per-team via a new by_org_user_period_team index, so each team's consumption is tracked independently. Budget warnings in the banner respect the selected team filter, while hard-block exceeded checks still evaluate across all teams to prevent bypass. Also navigates to dashboard on team switch.
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.
Summary
Closes #1452, closes #1450, closes #1456
Budget rules with
maxRequestslimits were not blocking users who exceeded the limit.incrementUsageLedgeronly recorded usage under the monthly period key, so daily/weekly budget rules always found zero usage and passedTest plan
maxRequests: 2, send 3 messages → verify 3rd is blockedmaxRequests: 5→ verify enforcement after 5 requestsnpx tsc --noEmitpassesnpm run lint --workspace=@tale/platformpassesnpx vitest run convex/governance/__tests__/)