fix(web): respect display currency in model pricing editor - #6060
Closed
zhangjk1207 wants to merge 1 commit into
Closed
fix(web): respect display currency in model pricing editor#6060zhangjk1207 wants to merge 1 commit into
zhangjk1207 wants to merge 1 commit into
Conversation
Contributor
WalkthroughThe model pricing editor now uses configured currency symbols and exchange rates, converts stored USD values for display, converts entered values back to USD on submission, and applies the same formatting to inputs and previews. Conversion helpers and bidirectional tests were added. ChangesCurrency-aware model pricing
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/default/src/features/system-settings/models/pricing-format.test.ts (2)
1-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Vitest instead of
node:testfor unit tests.The file imports from
node:testandnode:assert/strict, but the coding guidelines forweb/default/**/*.test.tsrequire Vitest. The.tsextension in the import path also suggests Node-native execution rather than Vitest/Vite resolution.As per coding guidelines: "Write unit tests for utility functions and pure logic with Vitest."
♻️ Migrate to Vitest
-import assert from 'node:assert/strict' -import { describe, test } from 'node:test' +import { describe, it, expect } from 'vitest' import { formatDisplayPriceFromUSD, formatUSDPriceFromDisplay, -} from './pricing-format.ts' +} from './pricing-format' describe('model pricing display currency conversion', () => { - test('converts stored USD model prices to the configured display currency', () => { - assert.equal(formatDisplayPriceFromUSD('3', 7), '21') + it('converts stored USD model prices to the configured display currency', () => { + expect(formatDisplayPriceFromUSD('3', 7)).toBe('21') }) - test('converts display currency model prices back to stored USD values', () => { - assert.equal(formatUSDPriceFromDisplay('21', 7), '3') + it('converts display currency model prices back to stored USD values', () => { + expect(formatUSDPriceFromDisplay('21', 7)).toBe('3') }) })🤖 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 `@web/default/src/features/system-settings/models/pricing-format.test.ts` around lines 1 - 17, Replace the node:test and node:assert/strict imports in the pricing conversion test suite with Vitest APIs, using describe, test, and expect. Update the assertions in the model pricing display currency conversion tests from assert.equal to expect(...).toBe(...), and remove the .ts extension from the pricing-format import so Vitest/Vite resolves it normally.Source: Coding guidelines
9-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd edge-case tests for conversion helpers.
The two tests only cover the happy path with exact round-trip values. Consider adding cases for: null/undefined/empty-string inputs (should return
''), zero or negative exchange rates (should default to rate1), and non-numeric string inputs.♻️ Suggested edge-case tests
test('converts display currency model prices back to stored USD values', () => { assert.equal(formatUSDPriceFromDisplay('21', 7), '3') }) + + test('returns empty string for null or invalid inputs', () => { + assert.equal(formatDisplayPriceFromUSD(null, 7), '') + assert.equal(formatDisplayPriceFromUSD('', 7), '') + assert.equal(formatDisplayPriceFromUSD('abc', 7), '') + assert.equal(formatUSDPriceFromDisplay(undefined, 7), '') + }) + + test('defaults exchange rate to 1 when zero or negative', () => { + assert.equal(formatDisplayPriceFromUSD('3', 0), '3') + assert.equal(formatDisplayPriceFromUSD('3', -1), '3') + assert.equal(formatUSDPriceFromDisplay('3', 0), '3') + }) })🤖 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 `@web/default/src/features/system-settings/models/pricing-format.test.ts` around lines 9 - 16, Add edge-case coverage for formatDisplayPriceFromUSD and formatUSDPriceFromDisplay: verify null, undefined, and empty-string inputs return '', zero and negative exchange rates behave as rate 1, and non-numeric strings are handled as expected. Keep the existing happy-path tests and add focused cases for each conversion helper.
🤖 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 `@web/default/src/features/system-settings/models/model-pricing-inputs.tsx`:
- Around line 91-101: Update the conditional rendering around the helper text in
the model pricing input component: keep the currency-specific “USD price per 1M
tokens.” text gated by `!props.hideUnitDescription`, but render the “Disabled
lanes are omitted on save.” hint whenever `props.enabled` is false, regardless
of `hideUnitDescription`.
---
Nitpick comments:
In `@web/default/src/features/system-settings/models/pricing-format.test.ts`:
- Around line 1-17: Replace the node:test and node:assert/strict imports in the
pricing conversion test suite with Vitest APIs, using describe, test, and
expect. Update the assertions in the model pricing display currency conversion
tests from assert.equal to expect(...).toBe(...), and remove the .ts extension
from the pricing-format import so Vitest/Vite resolves it normally.
- Around line 9-16: Add edge-case coverage for formatDisplayPriceFromUSD and
formatUSDPriceFromDisplay: verify null, undefined, and empty-string inputs
return '', zero and negative exchange rates behave as rate 1, and non-numeric
strings are handled as expected. Keep the existing happy-path tests and add
focused cases for each conversion helper.
🪄 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: CHILL
Plan: Pro
Run ID: 66562da1-a6c7-4fea-af01-865352eaa5bc
📒 Files selected for processing (5)
web/default/src/features/system-settings/models/model-pricing-core.tsweb/default/src/features/system-settings/models/model-pricing-inputs.tsxweb/default/src/features/system-settings/models/model-pricing-sheet.tsxweb/default/src/features/system-settings/models/pricing-format.test.tsweb/default/src/features/system-settings/models/pricing-format.ts
Comment on lines
+91
to
+101
| prefix={props.prefix} | ||
| suffix={props.suffix} | ||
| onChange={props.onChange} | ||
| /> | ||
| <p className='text-muted-foreground text-xs'> | ||
| {props.enabled | ||
| ? t('USD price per 1M tokens.') | ||
| : t('Disabled lanes are omitted on save.')} | ||
| </p> | ||
| {!props.hideUnitDescription && ( | ||
| <p className='text-muted-foreground text-xs'> | ||
| {props.enabled | ||
| ? t('USD price per 1M tokens.') | ||
| : t('Disabled lanes are omitted on save.')} | ||
| </p> | ||
| )} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
hideUnitDescription also hides the non-currency-specific "Disabled lanes" hint.
When hideUnitDescription is true (non-USD currency), the entire <p> is omitted, including t('Disabled lanes are omitted on save.'). That message is not currency-specific and should remain visible so non-USD users still understand what happens when a lane is disabled.
🐛 Proposed fix: separate the two conditions
/>
- {!props.hideUnitDescription && (
- <p className='text-muted-foreground text-xs'>
- {props.enabled
- ? t('USD price per 1M tokens.')
- : t('Disabled lanes are omitted on save.')}
- </p>
- )}
+ {props.enabled && !props.hideUnitDescription && (
+ <p className='text-muted-foreground text-xs'>
+ {t('USD price per 1M tokens.')}
+ </p>
+ )}
+ {!props.enabled && (
+ <p className='text-muted-foreground text-xs'>
+ {t('Disabled lanes are omitted on save.')}
+ </p>
+ )}
</SettingsControlGroup>📝 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
| prefix={props.prefix} | |
| suffix={props.suffix} | |
| onChange={props.onChange} | |
| /> | |
| <p className='text-muted-foreground text-xs'> | |
| {props.enabled | |
| ? t('USD price per 1M tokens.') | |
| : t('Disabled lanes are omitted on save.')} | |
| </p> | |
| {!props.hideUnitDescription && ( | |
| <p className='text-muted-foreground text-xs'> | |
| {props.enabled | |
| ? t('USD price per 1M tokens.') | |
| : t('Disabled lanes are omitted on save.')} | |
| </p> | |
| )} | |
| /> | |
| {props.enabled && !props.hideUnitDescription && ( | |
| <p className='text-muted-foreground text-xs'> | |
| {t('USD price per 1M tokens.')} | |
| </p> | |
| )} | |
| {!props.enabled && ( | |
| <p className='text-muted-foreground text-xs'> | |
| {t('Disabled lanes are omitted on save.')} | |
| </p> | |
| )} |
🤖 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 `@web/default/src/features/system-settings/models/model-pricing-inputs.tsx`
around lines 91 - 101, Update the conditional rendering around the helper text
in the model pricing input component: keep the currency-specific “USD price per
1M tokens.” text gated by `!props.hideUnitDescription`, but render the “Disabled
lanes are omitted on save.” hint whenever `props.enabled` is false, regardless
of `hideUnitDescription`.
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
2 participants
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.
Important
📝 变更描述 / Description
修复新版 UI 的“系统设置 -> 模型定价”编辑器未遵循额度展示货币的问题。
编辑已有模型时，已存储的 USD 价格会按配置汇率转换为展示货币；输入、扩展价格通道和预览使用当前货币的符号与单位；保存时将展示货币价格换回 USD，保持既有后端存储格式和计费逻辑兼容。
实现过程使用 AI 辅助，并基于实际复现、代码逻辑和本地验证整理本 PR。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
Summary by CodeRabbit
New Features
Bug Fixes
Tests