[codex] Fix model square group pricing display - #5943
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull-request workflow runs frontend tests when files under ChangesFrontend test automation
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/default/src/features/pricing/components/model-card.tsx (2)
92-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a component-level regression test for group-based price switching.
Since this PR's core objective is fixing "model square displays wrong price after changing group," an RTL test rendering
ModelCardwith differentselectedGroup/groupRatioprops and asserting the displayed price text changes accordingly would directly protect against this regression recurring. Per path instructions, tests should "focus on user interactions and behavior rather than implementation details" and "protect real user behavior... or explicit regressions."🤖 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/pricing/components/model-card.tsx` around lines 92 - 198, Add a component-level RTL regression test for ModelCard that renders with different selectedGroup and groupRatio props and verifies the displayed price text updates when the group changes. Use the ModelCard component and the price-rendering branches driven by formatPrice/formatRequestPrice to assert user-visible behavior rather than internals, so the “wrong price after changing group” bug stays covered.Source: Path instructions
92-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting
priceContentcomputation into a helper/hook.This branch chain now spans ~107 lines and repeats near-identical 8-argument
formatPrice/formatRequestPricecalls for input/output/cache. Per path instructions, "Keep functions readable by controlling cyclomatic complexity, splitting complex logic into small functions" and "Keep files reasonably small; when a single file grows beyond about 200 lines, consider extracting subcomponents or custom hooks." This file is already near/over that threshold with this addition.Extracting a small
usePriceContent(model, options)hook (or aformatModelPriceLine(type, label)helper to remove the triple-duplicated argument list) would reduceModelCard's complexity and centralize this rendering logic for reuse (e.g., in future price-display surfaces).🤖 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/pricing/components/model-card.tsx` around lines 92 - 198, The `ModelCard` price rendering logic is too large and duplicated, so extract the `priceContent` branch chain into a small helper or hook such as `usePriceContent` or a formatting helper around `formatPrice`/`formatRequestPrice`. Move the repeated input/output/cache rendering and shared argument list into a reusable function, keeping `ModelCard` focused on layout and making the pricing logic easier to read and maintain.Source: Path instructions
web/default/src/features/pricing/lib/dynamic-price.ts (1)
68-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate ratio-selection logic vs.
price.ts'sgetDisplayGroupRatio.This function reimplements the same "selected-group-or-min-ratio" algorithm already added in
price.ts'sgetDisplayGroupRatio(lines 77-87 of that file), just with amodel-based signature instead of(enableGroups, groupRatio). Both compute the min-ratio fallback independently:if (selectedGroup && groups.includes(selectedGroup)) { return ratios[selectedGroup] ?? 1 } ... inline min-ratio loop ...vs.
price.ts's delegation togetMinGroupRatio. Keeping two independent implementations risks them drifting out of sync (e.g., one gets a bugfix the other doesn't). Consider exportinggetDisplayGroupRatiofromprice.tsand having this function deriveenableGroups/ratiosfrommodelbefore delegating to it.♻️ Suggested consolidation
+import { getDisplayGroupRatio } from './price' + export function getDynamicDisplayGroupRatio( model: PricingModel, selectedGroup?: string, displayGroupRatio?: Record<string, number> ): number { const groups = Array.isArray(model.enable_groups) ? model.enable_groups : [] const ratios = displayGroupRatio ?? model.group_ratio ?? {} - if (groups.length === 0) return 1 - if (selectedGroup && groups.includes(selectedGroup)) { - return ratios[selectedGroup] ?? 1 - } - - let minRatio = Number.POSITIVE_INFINITY - for (const group of groups) { - const ratio = ratios[group] - if (ratio !== undefined && ratio < minRatio) { - minRatio = ratio - } - } - - return minRatio === Number.POSITIVE_INFINITY ? 1 : minRatio + if (groups.length === 0) return 1 + return getDisplayGroupRatio(groups, ratios, selectedGroup) }(requires exporting
getDisplayGroupRatiofromprice.ts)🤖 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/pricing/lib/dynamic-price.ts` around lines 68 - 89, The ratio-selection logic in getDynamicDisplayGroupRatio duplicates the same selected-group-or-min-ratio behavior already implemented in price.ts’s getDisplayGroupRatio, so consolidate the shared logic there instead of maintaining two versions. Refactor getDynamicDisplayGroupRatio to derive enableGroups and ratios from model, then delegate to the exported getDisplayGroupRatio from price.ts (or a shared helper used by it), keeping the model-specific wrapper thin and avoiding a separate min-ratio loop.
🤖 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/pricing/lib/price.test.ts`:
- Around line 1-2: The pricing test in price.test.ts is using node:test and
node:assert, but it needs to follow the repo’s Vitest setup so it actually runs.
Update the test in price.test.ts to use vitest imports like
describe/test/expect, and add a web/default test script so the package’s tests
are picked up by CI and local runs.
---
Nitpick comments:
In `@web/default/src/features/pricing/components/model-card.tsx`:
- Around line 92-198: Add a component-level RTL regression test for ModelCard
that renders with different selectedGroup and groupRatio props and verifies the
displayed price text updates when the group changes. Use the ModelCard component
and the price-rendering branches driven by formatPrice/formatRequestPrice to
assert user-visible behavior rather than internals, so the “wrong price after
changing group” bug stays covered.
- Around line 92-198: The `ModelCard` price rendering logic is too large and
duplicated, so extract the `priceContent` branch chain into a small helper or
hook such as `usePriceContent` or a formatting helper around
`formatPrice`/`formatRequestPrice`. Move the repeated input/output/cache
rendering and shared argument list into a reusable function, keeping `ModelCard`
focused on layout and making the pricing logic easier to read and maintain.
In `@web/default/src/features/pricing/lib/dynamic-price.ts`:
- Around line 68-89: The ratio-selection logic in getDynamicDisplayGroupRatio
duplicates the same selected-group-or-min-ratio behavior already implemented in
price.ts’s getDisplayGroupRatio, so consolidate the shared logic there instead
of maintaining two versions. Refactor getDynamicDisplayGroupRatio to derive
enableGroups and ratios from model, then delegate to the exported
getDisplayGroupRatio from price.ts (or a shared helper used by it), keeping the
model-specific wrapper thin and avoiding a separate min-ratio loop.
🪄 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: aaea7169-7bdd-4561-b908-b34253414f73
📒 Files selected for processing (8)
web/default/src/features/pricing/components/model-card-grid.tsxweb/default/src/features/pricing/components/model-card.tsxweb/default/src/features/pricing/components/pricing-columns.tsxweb/default/src/features/pricing/components/pricing-table.tsxweb/default/src/features/pricing/index.tsxweb/default/src/features/pricing/lib/dynamic-price.tsweb/default/src/features/pricing/lib/price.test.tsweb/default/src/features/pricing/lib/price.ts
| import assert from 'node:assert/strict' | ||
| import { describe, test } from 'node:test' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== repo files of interest =='
git ls-files | rg '(^|/)(package.json|bunfig\.toml|vitest\.config\.[cm]?[jt]s|vite\.config\.[cm]?[jt]s|tsconfig.*json|AGENTS\.md|price\.test\.ts|price\.ts)$'
echo
echo '== package.json scripts =='
sed -n '1,220p' package.json
echo
echo '== web/default package.json scripts (if present) =='
if [ -f web/default/package.json ]; then
sed -n '1,220p' web/default/package.json
fi
echo
echo '== bun config =='
if [ -f bunfig.toml ]; then
sed -n '1,200p' bunfig.toml
fi
echo
echo '== vitest / test config candidates =='
for f in $(git ls-files | rg '(^|/)(vitest\.config\.[cm]?[jt]s|vite\.config\.[cm]?[jt]s|package.json|bunfig\.toml)$'); do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo
echo '== feature test file =='
sed -n '1,220p' web/default/src/features/pricing/lib/price.test.ts
echo
echo '== search for node:test and vitest usage in tests =='
rg -n --glob '**/*.test.ts' --glob '**/*.test.tsx' "from 'node:test'|from 'vitest'|node:assert|assert\.equal|expect\(" web/default/srcRepository: QuantumNous/new-api
Length of output: 575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== web/default/package.json =='
sed -n '1,240p' web/default/package.json
echo
echo '== web/default/AGENTS.md =='
sed -n '1,260p' web/default/AGENTS.md
echo
echo '== CI/workflow files mentioning test or bun =='
git ls-files '.github/workflows/*' | while read -r f; do
if rg -n "bun|vitest|test" "$f" >/dev/null; then
echo "--- $f"
rg -n "bun|vitest|test" "$f"
fi
done
echo
echo '== search for test scripts and vitest config under web/default =='
rg -n --glob 'web/default/**' "vitest|bun run test|bun test|test:" web/defaultRepository: QuantumNous/new-api
Length of output: 13813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== web/package.json =='
sed -n '1,240p' web/package.json
echo
echo '== .github/workflows/pr-check.yml =='
sed -n '1,240p' .github/workflows/pr-check.yml
echo
echo '== search for test commands in workflows =='
rg -n "bun run test|bun test|vitest|node:test|npm test|pnpm test|yarn test" .github/workflows web/package.json web/default/package.jsonRepository: QuantumNous/new-api
Length of output: 2072
Wire web/default tests into Vitest
price.test.ts isn’t picked up by any current test script or CI job, so it won’t run as-is. Add a test script for web/default and switch this file to vitest/expect to match the repo’s test convention.
🤖 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/pricing/lib/price.test.ts` around lines 1 - 2, The
pricing test in price.test.ts is using node:test and node:assert, but it needs
to follow the repo’s Vitest setup so it actually runs. Update the test in
price.test.ts to use vitest imports like describe/test/expect, and add a
web/default test script so the package’s tests are picked up by CI and local
runs.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/frontend-test.yml (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Bun version.
bun-version: latestchanges the CI runtime without a repository change. Use the version declared by the repository, or pin an exact version and update it intentionally.🤖 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 @.github/workflows/frontend-test.yml around lines 17 - 19, Update the bun-version setting in the setup-bun step to use the repository’s declared Bun version, or replace latest with an exact intentionally maintained version. Keep the existing oven-sh/setup-bun action and other workflow settings unchanged.
🤖 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 @.github/workflows/frontend-test.yml:
- Line 16: Update the actions/checkout step in the workflow to set
persist-credentials to false, unless a later step requires authenticated Git
operations; leave the existing checkout action version unchanged.
- Around line 5-7: Update the pull_request path filters in the frontend-test
workflow to include web/package.json and web/bun.lock alongside the existing
web/default and workflow paths, so workspace installation input changes trigger
the job.
---
Nitpick comments:
In @.github/workflows/frontend-test.yml:
- Around line 17-19: Update the bun-version setting in the setup-bun step to use
the repository’s declared Bun version, or replace latest with an exact
intentionally maintained version. Keep the existing oven-sh/setup-bun action and
other workflow settings unchanged.
🪄 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 Plus
Run ID: c5a8ebd0-0ddb-4aa2-b98d-63542818c6f4
📒 Files selected for processing (1)
.github/workflows/frontend-test.yml
| test-default-frontend: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow =="
if [ -f .github/workflows/frontend-test.yml ]; then
nl -ba .github/workflows/frontend-test.yml | sed -n '1,120p'
else
echo "missing .github/workflows/frontend-test.yml"
fi
echo
echo "== checkout uses in workflows =="
rg -n "actions/checkout|persist-credentials|git config|GITHUB_TOKEN|bun install|bun test" .github/workflows || trueRepository: QuantumNous/new-api
Length of output: 212
🌐 Web query:
actions checkout persist-credentials default true README
💡 Result:
In the actions/checkout GitHub Action, the persist-credentials input determines whether the authentication token or SSH key used for the checkout is configured within the local git config [1][2]. By default, persist-credentials is set to true [1][3][4]. When enabled, this allows your subsequent workflow steps to run authenticated git commands (such as git push or git fetch) without requiring additional configuration [1][2]. The token or key is automatically removed during the post-job cleanup phase [1][2]. If you wish to prevent the credentials from being configured in the local git config, you can explicitly set persist-credentials: false in your workflow file [1][2]. Note: Recent improvements to the action (as of v6.0.0 and later) have enhanced security by storing these credentials in a separate file under $RUNNER_TEMP rather than directly in.git/config, though the default behavior of enabling persistence remains [1][5]. While there have been discussions and proposals regarding changing this default to false to reduce security risks, it remains true in the current implementation [6][3][4].
Citations:
- 1: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 2: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 3: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 4: https://github.com/actions/checkout/blob/72f2cec99f417b1a1c5e2e88945068983b7965f9/action.yml
- 5: https://github.com/actions/checkout/tree/v6.0.0
- 6: Change the default value of persist-credentials to false actions/checkout#1687
🏁 Script executed:
#!/bin/bash
set -u
echo "== workflow =="
if [ -f .github/workflows/frontend-test.yml ]; then
awk '{printf "%4d:%s\n", NR, $0}' .github/workflows/frontend-test.yml | sed -n '1,140p'
else
echo "missing .github/workflows/frontend-test.yml"
fi
echo
echo "== checkout uses in workflows =="
rg -n "actions/checkout|persist-credentials|git config|GITHUB_TOKEN|bun install|bun test" .github/workflows || trueRepository: QuantumNous/new-api
Length of output: 2965
Disable checkout credential persistence.
actions/checkout persists checkout credentials by default. Add persist-credentials: false unless this workflow needs later authenticated Git operations.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 16-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/frontend-test.yml at line 16, Update the actions/checkout
step in the workflow to set persist-credentials to false, unless a later step
requires authenticated Git operations; leave the existing checkout action
version unchanged.
Source: Linters/SAST tools
1 participant
Important
沐峰 <zhukangfeng.zkf@antgroup.com>, which is not among the recurring top authors seen ingit log(for example CaIon / Calcium-Ion, JustSong, t0ng7u, Seefs). This code was AI-assisted by Codex automation.📝 变更描述 / Description
新版模型广场按分组筛选时，卡片和表格仍使用最低分组倍率展示价格，导致用户选择高倍率分组后看到的价格不变。
本 PR 将当前选中的分组和
/api/pricing返回的全局group_ratio传入模型广场的卡片、表格和动态定价展示路径。未选择具体分组时仍保留原有最低可用分组价格展示；选择具体分组时展示该分组倍率下的 token/request 价格。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
Summary by CodeRabbit