Skip to content

Apply Discounted Rate to Helicone#5086

Closed
colegottdank wants to merge 5 commits intomainfrom
discounted-rate
Closed

Apply Discounted Rate to Helicone#5086
colegottdank wants to merge 5 commits intomainfrom
discounted-rate

Conversation

@colegottdank
Copy link
Collaborator

No description provided.

@vercel
Copy link

vercel bot commented Oct 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
helicone Ready Ready Preview Comment Oct 21, 2025 3:55am
helicone-bifrost Ready Ready Preview Comment Oct 21, 2025 3:55am
helicone-eu Ready Ready Preview Comment Oct 21, 2025 3:55am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

@claude
Copy link
Contributor

claude bot commented Oct 21, 2025

Claude finished @colegottdank's task —— View job


PR Review: Apply Discounted Rate to Helicone

Todo List:

  • Read core implementation files to understand the feature
  • Analyze the critical SQL issue identified by Greptile
  • Review cost calculation logic for correctness
  • Check admin API security and validation
  • Review worker implementation for performance
  • Analyze UI changes for consistency
  • Provide comprehensive feedback and score

Analysis:

This PR implements organization-level pricing multipliers for endpoints. The implementation allows admins to set multipliers (0-2 range) for specific model:provider endpoint combinations, affecting cost calculations in the worker.

Score: 6/10 - Minor issues and improvements suggested, merge with consideration

Critical Issues Found:

❌ Greptile's SQL Issue is Invalid: After reviewing the code, Greptile's concern about heliconePricingMultiplier and SQL type conversion doesn't apply. The implementation uses endpointMultipliers (Record<string, number>) and correctly converts it to JSON using JSON.stringify().

Code Quality Issues:

  1. ✅ Security is Proper: The admin API correctly validates admin permissions via authCheckThrow() which checks the admins table - my initial concern was incorrect.

  2. ⚠️ Type Safety Concerns (valhalla/jawn/src/controllers/private/adminController.ts:1355):

    @Body() body: { endpointMultipliers: Record<string, number> }

    Should use a proper interface for better type safety:

    interface PricingConfigRequest {
      endpointMultipliers: Record<string, number>;
    }
  3. ⚠️ Inconsistent Typing (worker/src/lib/db/DBWrapper.ts:241):

    pricingConfig: org.data.pricing_config as { heliconePricingMultiplier?: number } | undefined,

    This type doesn't match the actual structure. Should be:

    pricingConfig: org.data.pricing_config as { endpointMultipliers?: Record<string, number> } | undefined,
  4. ⚠️ Error Handling Gap (worker/src/lib/HeliconeProxyRequest/ProxyForwarder.ts:43):

    function getPricingMultiplier(pricingConfig: any, endpointKey: string | null): number {

    Should handle malformed pricing_config JSON:

    function getPricingMultiplier(
      pricingConfig: { endpointMultipliers?: Record<string, number> } | null,
      endpointKey: string | null
    ): number {
      try {
        if (!pricingConfig?.endpointMultipliers || !endpointKey) {
          return 1.0;
        }
        return pricingConfig.endpointMultipliers[endpointKey] ?? 1.0;
      } catch (error) {
        console.error('Error parsing pricing config:', error);
        return 1.0;
      }
    }

Performance Considerations:

  1. ⚠️ Database Query Optimization: The pricing_config is fetched on every request. Consider adding caching if this becomes a bottleneck.

  2. ✅ Cost Calculation Logic: The cost calculation logic in calculateModelCostBreakdown correctly applies the multiplier only to the final total cost, preserving individual cost breakdowns for transparency.

Implementation Quality:

  1. ✅ Proper Validation: Input validation correctly restricts multipliers to 0-2 range and validates endpoint key format.

  2. ✅ UI Implementation: The admin UI properly handles CRUD operations for endpoint multipliers with confirmation dialogs and error handling.

  3. ✅ Database Migration: Simple and appropriate migration adding nullable JSONB column.

Suggestions Summary:

  • Fix type inconsistencies between DBWrapper and actual pricing config structure
  • Add proper interfaces instead of using any types
  • Improve error handling in getPricingMultiplier function
  • Consider caching pricing_config if performance becomes an issue

The implementation is solid overall with proper security, validation, and cost calculation logic. The issues are primarily around type safety and error handling rather than critical functionality problems.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Summary

Implements organization-level pricing multipliers for Helicone provider costs. Adds a new pricing_config JSONB column to the organization table, an admin API endpoint to configure the multiplier (0-2 range), and applies the multiplier during cost calculation in the worker for requests using the "helicone" provider.

Key Changes:

  • Database migration adds nullable pricing_config JSONB column
  • Admin API endpoint validates and stores heliconePricingMultiplier (0-2 range)
  • Cost calculation package accepts optional pricingMultiplier parameter
  • Worker applies multiplier only when provider is "helicone"
  • Admin UI provides interface to view and edit pricing multiplier per organization

Issue Found:

  • SQL query uses $1::text::jsonb which stores the multiplier as a JSON string instead of a JSON number, causing type inconsistency

Confidence Score: 3/5

  • Safe to merge after fixing the SQL type conversion issue
  • The implementation is well-structured with proper validation and scoped changes, but contains a critical SQL type conversion bug that stores numbers as JSON strings instead of JSON numbers. This could cause runtime type errors or incorrect behavior when the stored value is read back.
  • Pay close attention to valhalla/jawn/src/controllers/private/adminController.ts - the SQL query needs to be fixed to store the multiplier as a JSON number rather than a string

Important Files Changed

File Analysis

Filename Score Overview
supabase/migrations/20251021005851_org_pricing_config.sql 5/5 Added nullable pricing_config jsonb column to organization table
valhalla/jawn/src/controllers/private/adminController.ts 4/5 Added POST endpoint to update org pricing config with validation for multiplier between 0 and 2
packages/cost/models/calculate-cost.ts 5/5 Added pricingMultiplier parameter to cost calculation, multiplying final total cost
worker/src/lib/db/DBWrapper.ts 5/5 Added pricingConfig to AuthParams, extracting from organization pricing_config
worker/src/lib/HeliconeProxyRequest/ProxyForwarder.ts 4/5 Applied pricing multiplier to cost calculations only for provider="helicone"
web/components/templates/admin/orgSearch.tsx 5/5 Added HeliconePricingSection component with UI to view/edit pricing multiplier

Sequence Diagram

sequenceDiagram
    participant Admin as Admin UI
    participant Jawn as Jawn API
    participant DB as PostgreSQL
    participant Worker as Worker
    participant Cost as Cost Package

    Note over Admin,Cost: Configuration Flow
    Admin->>Jawn: POST /v1/admin/org/{orgId}/pricing-config
    Note right of Jawn: Validate 0 ≤ multiplier ≤ 2
    Jawn->>DB: UPDATE organization SET pricing_config
    DB-->>Jawn: Success
    Jawn-->>Admin: 200 OK

    Note over Admin,Cost: Request Processing Flow
    Worker->>DB: Get organization data
    DB-->>Worker: org with pricing_config
    Note right of Worker: Extract heliconePricingMultiplier
    Worker->>Worker: Process request & get response
    Worker->>Cost: calculateModelCostBreakdown()
    Note right of Worker: pricingMultiplier = org.pricing_config.heliconePricingMultiplier
    Note right of Worker: Only if provider === "helicone"
    Cost->>Cost: Calculate base costs
    Cost->>Cost: totalCost *= pricingMultiplier
    Cost-->>Worker: CostBreakdown with adjusted cost
    Worker->>Worker: Log request with adjusted cost
Loading

16 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +1370 to +1371
`UPDATE organization SET pricing_config = jsonb_set(COALESCE(pricing_config, '{}'), '{heliconePricingMultiplier}', $1::text::jsonb) WHERE id = $2`,
[heliconePricingMultiplier.toString(), orgId]
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: $1::text::jsonb converts the number to a string first, storing it as a JSON string ("1.5") instead of a JSON number (1.5). This causes type inconsistency since the code expects a number.

Suggested change
`UPDATE organization SET pricing_config = jsonb_set(COALESCE(pricing_config, '{}'), '{heliconePricingMultiplier}', $1::text::jsonb) WHERE id = $2`,
[heliconePricingMultiplier.toString(), orgId]
`UPDATE organization SET pricing_config = jsonb_set(COALESCE(pricing_config, '{}'), '{heliconePricingMultiplier}', to_jsonb($1::float)) WHERE id = $2`,
[heliconePricingMultiplier, orgId]
Prompt To Fix With AI
This is a comment left during a code review.
Path: valhalla/jawn/src/controllers/private/adminController.ts
Line: 1370:1371

Comment:
**syntax:** `$1::text::jsonb` converts the number to a string first, storing it as a JSON string (`"1.5"`) instead of a JSON number (`1.5`). This causes type inconsistency since the code expects a number.

```suggestion
      `UPDATE organization SET pricing_config = jsonb_set(COALESCE(pricing_config, '{}'), '{heliconePricingMultiplier}', to_jsonb($1::float)) WHERE id = $2`,
      [heliconePricingMultiplier, orgId]
```

How can I resolve this? If you propose a fix, please make it concise.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant