fix: use locale-aware decimal separator in trace duration formatting#294
Conversation
Pipe pretty-ms output through a string replace so the decimal separator matches the browser locale, eliminating ambiguity with locale-specific thousands separators in token counts. Closes #292 Co-authored-by: Heinrich Wendel <heiwen@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughA helper function was added to derive locale-specific decimal separators using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/console/app/routes/_shell.agent.$agentSlug.branch.$branchSlug.traces/utils.ts (1)
4-10: Consider memoizing the decimal separator.
getDecimalSeparator()creates a newIntl.NumberFormatinstance on each invocation. Since the locale doesn't change during a session, memoizing the result avoids repeated object allocations—particularly relevant in list views that render many trace items.♻️ Proposed memoization
-function getDecimalSeparator(): string { - return ( - new Intl.NumberFormat(undefined) - .formatToParts(1.1) - .find((p) => p.type === "decimal")?.value ?? "." - ); -} +const getDecimalSeparator = (() => { + let cached: string | undefined; + return (): string => { + if (cached === undefined) { + cached = + new Intl.NumberFormat(undefined) + .formatToParts(1.1) + .find((p) => p.type === "decimal")?.value ?? "."; + } + return cached; + }; +})();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/console/app/routes/_shell.agent`.$agentSlug.branch.$branchSlug.traces/utils.ts around lines 4 - 10, getDecimalSeparator currently allocates a new Intl.NumberFormat on every call; memoize its result by computing the decimal separator once (e.g., a module-level constant or a lazy-initialized variable) and return that cached value on subsequent calls to avoid repeated Intl.NumberFormat creation in hot paths; update the getDecimalSeparator function to use the cached value while preserving the existing logic (calling Intl.NumberFormat(undefined).formatToParts(1.1).find(...) ?? ".") and the same fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/console/app/routes/_shell.agent`.$agentSlug.branch.$branchSlug.traces/utils.ts:
- Around line 4-10: getDecimalSeparator currently allocates a new
Intl.NumberFormat on every call; memoize its result by computing the decimal
separator once (e.g., a module-level constant or a lazy-initialized variable)
and return that cached value on subsequent calls to avoid repeated
Intl.NumberFormat creation in hot paths; update the getDecimalSeparator function
to use the cached value while preserving the existing logic (calling
Intl.NumberFormat(undefined).formatToParts(1.1).find(...) ?? ".") and the same
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e70f6919-fcec-4307-bd82-f1c414c6fd79
📒 Files selected for processing (1)
apps/console/app/routes/_shell.agent.$agentSlug.branch.$branchSlug.traces/utils.ts
Use
Intl.NumberFormatto detect the browser's decimal separator and replace the hardcoded.frompretty-msoutput, so duration and token count formatting use consistent locale conventions.Closes #292
Generated with Claude Code
Summary by CodeRabbit