Show model cost tiers in the LLM picker - #5659
Conversation
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 54m 12s ⏱️ -28s Results for commit 333af8b. ± Comparison against earlier commit 9dbab75. Realm Server Test Results 1 files ± 0 1 suites ±0 14m 34s ⏱️ + 3m 16s Results for commit 333af8b. ± Comparison against earlier commit 9dbab75. For more details on these errors, see this check. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dbab7557b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let n = parseFloat(price); | ||
| return Number.isFinite(n) ? n : 'invalid'; |
There was a problem hiding this comment.
Reject partially numeric pricing strings
When OpenRouter supplies a malformed value with a numeric prefix, such as "0.000003 USD" or "1x", parseFloat accepts the prefix and the picker displays a potentially incorrect tier instead of treating the price as unknown as intended. Parse the entire value (for example with Number) and reject negative or otherwise invalid prices before calculating the tier.
Useful? React with 👍 / 👎.
lukemelia
left a comment
There was a problem hiding this comment.
Any reason not to make this a computed on the OpenRouterModel card or the ModelConfiguration card?
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review focuses on the one open question on the PR — whether the tier derivation belongs on a card as a computed field — and on the two places this change could quietly go wrong: the reactivity design in room.gts, and the robustness of the pure tier math. The visual/markup change in llm-select.gts reads cleanly and I have nothing to add there.
Bottom line: The tier logic and picker wiring are sound and well-tested, and the restartableTask design correctly sidesteps a real reactivity trap rather than papering over it. Nothing I found blocks on its own; the requested-changes call is the maintainer's, and it turns on the architectural question below. Two small items: a robustness gap in parsePrice (inline), and the branch currently has a merge conflict in environment.js (GitHub reports the PR as un-mergeable).
Answering the open review — "any reason not to make this a computed on OpenRouterModel or ModelConfiguration?"
The two candidate homes are not equivalent, and neither removes the machinery in room.gts. Laying out the mechanics so the decision is on the facts:
-
ModelConfiguration— not viable as acomputeVia.ModelConfiguration(inpackages/base/system-card.gts) carries onlymodelId,toolsSupported,reasoningEffort,inputModalities— no pricing, and no link toOpenRouterModel. The two cards are correlated by themodelIdstring alone. A computed there would have to do an async cross-realm lookup to find the matching catalog card, whichcomputeViacan't express. -
OpenRouterModel— idiomatic and a genuine improvement for the math. Pricing lives on this card, and it already computestoolsSupported,inputModalities, andcardTitleviacomputeViafrom its own fields, so acostTier/costTierLabelcomputed alongside them is right at home. The win is real: the tier becomes an indexed search-doc field (queryable/sortable, and the ~40 lines ofmodel-cost.tsderivation move onto the card and out of the host), and the host's search would just readinstance.costTierinstead of readingpricingand computing. -
But it does not eliminate the task/search in
room.gts. The reactivity problem this PR solves is about runningstore.searchinside a reactive computation — not about where the tier arithmetic lives. Because picker rows areModelConfigurationcards / fallback id strings (notOpenRouterModelcards) and the only correlation is themodelIdstring with no link, the host still has to search the OpenRouter realm bymodelIdand join client-side. So therestartableTask+ content-key gating stays regardless of where the derivation lives.
Net: a computed on OpenRouterModel is the better home for the derivation and matches the existing computeds; a computed on ModelConfiguration isn't achievable without a schema change (a linksTo OpenRouterModel). The reactivity design is orthogonal to that choice and is sound either way. My suggestion, for what it's worth: move the tier math to an OpenRouterModel computed (delete model-cost.ts's role in the host, have the task read the computed field), and keep the task exactly as written.
What lands right
- The reactivity design is the hard part and it's done correctly — not a
trackedFunctionthat would loop forever. Detail and the two load-bearing invariants are in the inline note on the task. - Graceful degradation is real, not asserted. Unconfigured realm, uncatalogued model, and a failing catalog search all resolve to "no badge," and the failure path deliberately avoids writing the tracked map so it can't spin into a retry loop while the catalog is down. The integration tests exercise all three branches.
- The pure tier logic is well-specified and well-tested — the 3:1 blend, inclusive fixed bounds, and the unknown-vs-Free distinction are all pinned by unit tests, and I re-derived the representative-model cases (e.g. sonnet
0.000003/0.000015→ blended6→$$$; o1 →26.25→$$$$) — they check out.
Recommendations
- Decide the architectural question above — an
OpenRouterModel.costTiercomputed is the clean move for the derivation;ModelConfigurationisn't viable without a link. (Body, above.) - Harden
parsePriceagainst numeric-prefixed / negative strings so it matches the file's own stated contract —Number()+ a>= 0guard. Non-blocking; not reachable with today's OpenRouter data. (Inline onmodel-cost.ts; this is the Codex P2, verified.) - Optionally add a negative-space unit test for a numeric-prefix price string, and a one-line reactivity caveat near
llmsForSelectMenu. (Inline notes.)
Adjacent, out of scope
- Merge conflict. The branch conflicts with
maininpackages/host/config/environment.js(both sides changed it since the branch point) — needs a merge/rebase before it can land. Purely mechanical, not a code issue. - Badge staleness within a session — the load's content key omits pricing, so a mid-session catalog price change won't refresh a badge. Reasonable for a cosmetic badge; called out in the inline note so it's a conscious trade.
Generated by Claude Code
| let n = parseFloat(price); | ||
| return Number.isFinite(n) ? n : 'invalid'; |
There was a problem hiding this comment.
[Claude Code 🤖] Non-blocking (robustness). parseFloat accepts a leading numeric prefix and returns a finite number for inputs the file header intends to treat as unknown — so parsePrice can classify a malformed price as valid and produce a wrong badge, which the top-of-file comment explicitly says it exists to prevent ("a present-yet-unparseable price makes the whole cost unknown — better no badge than a wrong tier").
Verified (Node):
parseFloat("0.000003 USD")→0.000003(finite → treated as a real price)parseFloat("1x")→1parseFloat("-0.001")→-0.001(finite) →blendedCostPerMilliongoes negative →modelCostTierhits theblended <= 0branch and rendersFreefor a negative price.
Number() rejects all three (Number("0.000003 USD") / Number("1x") → NaN), so the header's contract is met by construction. A >= 0 guard also closes the negative-price → Free path:
| let n = parseFloat(price); | |
| return Number.isFinite(n) ? n : 'invalid'; | |
| let n = Number(price); | |
| return Number.isFinite(n) && n >= 0 ? n : 'invalid'; |
'' and null are already returned as 'absent' above this line, so Number('') → 0 never reaches here.
Two caveats so this is fairly scoped:
- Not reachable with real data today. OpenRouter emits clean decimal strings (
"0.0000006","0"), so no live model trips this — it's defensive, not a live bug. This is the finding Codex raised; the above is the verification. - Consistent with existing style.
openrouter-model.gts'sformatPricealso usesparseFloat, so keepingparseFloathere wouldn't be out of place — but this file is the one that documents the stricter contract, which is whyNumber()fits it better.
Test-coverage note (non-blocking): the "unparseable → undefined" unit test only covers fully non-numeric strings ('n/a', 'abc', 'garbage'), which parseFloat already rejects. A numeric-prefix case like modelCostTierLabel('0.000003 USD', '0') → undefined would pin the exact behavior the header promises and would fail against the current parseFloat.
Generated by Claude Code
| // self-invalidate into an endless re-run and never settle. The task runs the | ||
| // search + field reads off the render's reactive path and only writes the | ||
| // tracked map the picker reads. | ||
| private loadModelCostTiers = restartableTask( |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation (nothing to change) + one limitation worth naming.
The choice to run this as a restartableTask gated on a plain (untracked) content key, rather than a trackedFunction/resource, is correct and worth keeping. this.store.search(...) hydrates cards — which mutates tracked store state — and the tier derivation then reads those hydrated cards' pricing fields; inside a reactive computation that read-after-write self-invalidates and never settles, so the badge would never appear. Two guard rails keep the current shape safe, and it's worth being explicit about them because they're load-bearing:
- The only tracked write,
this.modelCostTierByModelId = tiers, happens afterawait this.store.search(...)— i.e. in a later microtask — so even thoughllmsForSelectMenureads that same tracked field, the write lands in a separate runloop and can't trip Ember's backtracking-rerender assertion. lastModelCostTierKeyis deliberately untracked, so setting it on the synchronous render path (ensureModelCostTiersLoaded) invalidates nothing and can't retrigger the getter.
This stops being safe the moment either (a) llmsForSelectMenu reads any of the task's own tracked state (this.loadModelCostTiers.isRunning / .last / .performCount), or (b) anything on the synchronous path in ensureModelCostTiersLoaded writes tracked state. A one-line note to that effect near the getter would help the next editor who's tempted to add a spinner.
Limitation to name (non-blocking): the content key is realm + sorted model ids only — pricing is not part of it. So if the OpenRouter catalog updates a model's pricing while the model-id set stays the same, the badge won't refresh within the session (only a new/removed model id, or a page reload, forces a re-derive). Given badges are cosmetic and catalog pricing rarely moves mid-session, that's a reasonable trade — flagging it so it reads as a deliberate choice rather than an oversight.
Generated by Claude Code
Derive a glanceable cost tier (Free, $, $$, $$$, $$$$) for each AI model from its OpenRouter pricing and render it in the AI assistant model picker — in every row and beside the selected model. The tier is a blended 3:1 input:output price per million tokens, mapped to fixed dollar-sign bands. The picker looks it up by matching each model's modelId against the OpenRouter model catalog realm, degrading to no badge when the realm is unconfigured or a model has no catalog card. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Derive costTier/costTierLabel as computeVia fields on OpenRouterModel from its own pricing, so the tier is an indexed, queryable field and the LLM picker reads it off the catalog search rather than computing client-side. The pure tier math moves to @cardstack/runtime-common/model-cost (importable by the card runtime) and now rejects numeric-prefixed/negative price strings instead of surfacing a wrong tier. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9dbab75 to
333af8b
Compare
|
[Claude Code 🤖] Good call — I moved the derivation onto On the two candidate homes:
Also hardened |
|
[Claude Code 🤖] Posting a recommendation on behalf of Luke: On the To keep coverage, the idiomatic path is a co-located live test ( One important caveat if we go this way: the CI live-test job ( Not a blocker on this PR — the current |
lukemelia
left a comment
There was a problem hiding this comment.
See my comment about moving model-cost.ts' code out of runtime-common and into the realm.
Background and Goal
Give end users a glanceable sense of how expensive each AI model is, as a dollar-sign tier (
Free,$,$$,$$$,$$$$) shown in the AI assistant model picker (LLMSelect) — in every row and beside the selected model. Closes CS-12195.The tier is derived from the pricing that already lives on
OpenRouterModelcatalog cards; there was no cost-tier concept before this.Where to start
packages/host/app/utils/model-cost.ts— the pure tier logic. A blended 3:1 input:output price per million tokens (the industry-standard "blended" cost), mapped to fixed, human-round bands (≤$1→$,≤$5→$$,≤$20→$$$,>$20→$$$$;0→Free; unknown pricing → no badge). Fully unit-tested.packages/host/app/components/matrix/room.gts—pickerModelIds+loadModelCostTiersfill a@trackedmap the picker reads; see the reactivity note below.packages/host/app/components/ai-assistant/llm-select.gts—LLMOption.costTierLabel+ the badge markup/styling (a muted chip, distinct from the green per-token prices on the model cards).Key decisions and non-obvious mechanics
ModelConfigurationandOpenRouterModelare correlated only by themodelIdstring (no link), so the picker searches the OpenRouter catalog realm (config.resolvedOpenRouterRealmURL) bymodelIdand derives the tier client-side. Degrades to no badge when the realm is unconfigured or a model has no catalog card.trackedFunction/resource. The lookup can't run inside a reactive computation:store.searchhydrates cards (mutating tracked store state) and the tier derivation reads those cards' pricing fields, so the computation self-invalidates into an endless re-run and never settles (the picker just never shows a badge). Instead arestartableTaskgated on a content key (realm + sorted model ids) does the search off the render path and writes a@trackedmap — content-keyed so the live matrix churn behindpickerModelIds(a freshusedLLMsarray each sync) can't retrigger it. Verified working in the running app.ModelConfigurationvia itsmodelId), the uncatalogued-model case (no badge), and graceful degradation when the catalog search fails.Screenshot
🤖 Generated with Claude Code