fix(render): formatTokenCount boundary shows '100.0k' at 99999 (#1328) - #1348
fix(render): formatTokenCount boundary shows '100.0k' at 99999 (#1328)#1348griffinwork40 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Review of 121d51d2 | Change type: hotfix | Regime: light | Files: 2 | Lines: ~31
Decision: MERGE — 0 blocking (1 medium waived, 1 low).
Findings
1. medium · blocking: false · correctness · src/cli/render/stream-progress.ts:66 · ref: 121d51d2 · confidence: high
The .toFixed(1) branch now covers [1000, 10000). At tokens = 9999, (9999/1000).toFixed(1) evaluates to "10.0", producing "10.0k tokens" — the same class of rounding artifact this PR fixes at the 100k boundary, displaced one decade lower. Values in [9950, 9999] all display as "10.0k" while 10000 displays as "10k", creating a visible discontinuity.
The new test at line 63–69 asserts '10.0k tokens' for tokenCount: 9999, enshrining this as correct.
Evidence: if (tokens < 10_000) return \${(tokens / 1000).toFixed(1)}k tokens`;—(9999/1000).toFixed(1)→"10.0"` in JavaScript.
Suggestion: Either lower the .toFixed(1) ceiling further (e.g. to 1000, using Math.round for all [1000, 1M)) or document the accepted tradeoff. Follow-up issue is fine — the blast radius is cosmetic display only.
· waived: bounded, non-data-affecting cosmetic defect in a private formatting function; safe to land with a follow-up.
2. low · blocking: false · spec-compliance · src/cli/render/stream-progress.test.ts:63–69 · ref: 121d51d2 · confidence: high
The test "uses .toFixed(1) just below the 10k boundary" asserts 9999 → '10.0k tokens' as correct behavior, but this is the same rounding-artifact class the PR's stated intent was written to eliminate (at the 100k boundary). The stated fix target (no 100.0k at 99999) is fully achieved; this is an observation that the test locks in a related-but-displaced variant.
Dropped findings
- Test coverage — mid-range
[1000, 9999)gap: DROPPED (false-absent). A pre-existing test attokenCount: 1234(line 44) already covers this range and asserts'1.2k tokens'.
What was not checked
- Citations verified against branch HEAD
121d51d2. - Stated intent: PR #1328 title + body — spec-compliance assessed.
- Runtime behavior not exercised (no test execution).
- No perf benchmarks run (pure formatting, no load concern).
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (scheduled sweep), generated by the /review tool — a maintainer will follow up.
Prior Findings (Step 2.5)
No prior automated rounds on this PR — this is round 1.
Review: PR #1348 — fix(render): formatTokenCount boundary shows '100.0k' at 99999
Reviewed ref: 121d51d2a0b78117c116e5fd51d2f5520b1f45e0
Change type: hotfix | Regime: light (2 files, ~36 lines)
CI: ✅ All checks pass (Lint & Build, Test ubuntu/macos, PTY scrollback, Docs build, Publish bundle smoke)
Decision: MERGE (with follow-ups)
Severity arithmetic: 0 critical, 0 high → MERGE (with follow-ups); 1 medium advisory (non-blocking per floor policy), 3 low advisory.
The fix correctly eliminates the original defect (formatTokenCount(99999) yielding "100.0k tokens"). No critical or high findings — merging is safe.
Advisory Findings (non-blocking — suggest a follow-up issue)
MEDIUM — Correctness (advisory, non-blocking)
9999 → "10.0k tokens" is a rounding-up artefact at the new boundary
src/cli/render/stream-progress.ts:35 · file-state · ref 121d51d2
The fix moves the .toFixed(1) upper bound from 100_000 to 10_000. This correctly fixes 99999 → "100.0k", but introduces (9999/1000).toFixed(1) = "10.0" → "10.0k tokens" — a value below 10k rendered as if it were 10k. The new test explicitly accepts this as correct. It is a rounding artefact of the same class (.toFixed(1) rounding up to the next integer) but at a lower magnitude.
if (tokens < 10_000) return `${(tokens / 1000).toFixed(1)}k tokens`;
// (9999/1000).toFixed(1) = "10.0" → "10.0k tokens"Follow-up suggestion: Tighten the boundary so values that would round up to the next integer use Math.round instead — e.g. if (Math.round(tokens/1000) * 1000 >= 10_000) dispatch to the Math.round branch, or use a threshold like 9_950.
LOW — Spec Compliance (advisory)
Boundary choice of 10_000 not explained in PR description
src/cli/render/stream-progress.test.ts · diff-context · ref 121d51d2
The PR description states the goal but does not justify why 10_000 was the chosen boundary (vs. e.g. 9_950 or a value that avoids .toFixed(1) rounding up to the next integer). The test accepts 9999 → "10.0k" as correct without comment.
Follow-up suggestion: Add a one-line code comment in the source or test explaining the design decision (e.g., "below 10k, show one decimal; above 10k, round to nearest integer").
LOW — Test Coverage (advisory)
No mid-range sub-10k test case after the boundary change
src/cli/render/stream-progress.test.ts · diff-context · ref 121d51d2
The 4 new tests cover 9999, 10000, 99999, and 100000 — boundary values only. There is no test for a value like 5000 ("5.0k tokens") or 1500 ("1.5k tokens") that confirms .toFixed(1) works correctly through the full new [1000, 10000) range. The existing 1234 → "1.2k" test partially covers this but predates the boundary change.
Follow-up suggestion: Add tokenCount: 5000 → "5.0k tokens" test.
LOW — Correctness (advisory)
1000–1999 floor behaviour implicit, no test
src/cli/render/stream-progress.ts:34-35 · file-state · ref 121d51d2
The .toFixed(1) branch covers [1000, 10_000). There is no test asserting tokenCount: 1000 → "1.0k tokens". The lower floor of the k-range is implicit.
Follow-up suggestion: Add tokenCount: 1000 → "1.0k tokens" boundary test.
What Was Not Checked
- Citations verified inline against branch HEAD
121d51d2a0b78117c116e5fd51d2f5520b1f45e0. All 4 findings verified against file state at the reviewed ref. - Stated intent: PR #1348 title + body — spec-compliance assessed.
- Did not run the test suite locally (CI shows 15/15 passing per PR description and all GitHub Actions checks green).
- Did not review Telegram surface, other rendering paths, or adjacent token-counting logic outside
stream-progress.ts. - Security: no surface — pure formatting function on numeric input. API-compat:
formatTokenCountis unexported;streamProgressandStreamProgressSpecexports unchanged.
Fixes #1328.
formatTokenCount(99999)yielded"100.0k tokens"because the.toFixed(1)boundary was at 100,000. Lowered it to 10,000 so values >= 10k useMath.round(no trailing decimal).Changes
src/cli/render/stream-progress.ts-- boundary from100_000to10_000src/cli/render/stream-progress.test.ts-- 4 boundary test cases addedVerification
pnpm lintclean