Conversation
…cale option Replace hardcoded en-US locale with Intl.NumberFormat-derived locale that resolves decimal/thousands separators and currency symbol position from the runtime. Add locale option to FormatValueOptions, cached per locale:currency pair. Extract FormatValueMember type.
github-actions
bot
added
client:core
|
Claude finished @ovr's task in 2m 59s —— View job PR Review:
|
| Priority | Issue | Location |
|---|---|---|
| 🔴 High | Export is commented out — consumers can't use formatValue. Needs clarity on the access strategy. |
index.ts:913-914 |
| 🟡 Medium | parseNumber returns NaN for non-numeric strings — d3 will render literal "NaN" in the UI. Should handle gracefully. |
format.ts:70-76 |
| 🟡 Medium | Hardcoded grouping: [3] ignores locales with non-uniform grouping (e.g., en-IN uses [3, 2]). |
format.ts:24-26 |
| 🟢 Low | getCurrentLocale() allocates a new Intl.NumberFormat on every call — can be cached. |
format.ts:41-43 |
| 🟢 Low | Currency code leaks into cache key for non-currency formats, causing duplicate cache entries. | format.ts:119-122 |
| 🟢 Low | Missing test edge cases: NaN input, invalid dates, custom emptyPlaceholder, very large numbers. |
format.test.ts:84 |
Security
No concerns — this is a pure formatting utility with no network calls, DOM manipulation, or dynamic code execution.
Verdict
The core logic is sound and well-tested for happy paths. The commented-out export is the most important question — it determines whether this feature is actually shippable in this PR or a foundation for a follow-up. The NaN handling should also be addressed before merge to avoid rendering "NaN" in production UIs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| grouping: [3], | ||
| currency: symbolIsPrefix ? [currencySymbol, ''] : ['', currencySymbol], | ||
| }; |
There was a problem hiding this comment.
Hardcoded grouping: [3] ignores locale-specific grouping patterns.
Some locales use non-uniform digit grouping — for example, en-IN (Indian English) groups as 12,34,567 (groups of 2 after the first group of 3). The Intl API exposes this, but it's not being extracted here.
You could derive it from the formatToParts output by inspecting the integer parts between group separators, e.g.:
| grouping: [3], | |
| currency: symbolIsPrefix ? [currencySymbol, ''] : ['', currencySymbol], | |
| }; | |
| const integerParts = numParts.filter(p => p.type === 'integer').map(p => p.value.length); | |
| // Intl gives groups left-to-right; d3 wants them right-to-left from the decimal | |
| const grouping = integerParts.length > 1 ? integerParts.slice(1).reverse() : [3]; | |
| return { | |
| decimal: find('decimal') || '.', | |
| thousands: find('group') || ',', | |
| grouping, | |
| currency: symbolIsPrefix ? [currencySymbol, ''] : ['', currencySymbol], | |
| }; |
Not critical since most Western locales use [3], but worth considering for correctness.
| function parseNumber(value: any): number { | ||
| if (value === null || value === undefined) { | ||
| return 0; | ||
| } | ||
|
|
||
| return parseFloat(value); | ||
| } |
There was a problem hiding this comment.
parseNumber silently coerces non-numeric strings to NaN.
If someone passes value = "hello" with format: 'currency', parseFloat("hello") returns NaN, and d3-format will render "NaN". Consider returning the emptyPlaceholder or String(value) instead of formatting NaN:
function parseNumber(value: any): number | null {
if (value === null || value === undefined) {
return null;
}
const n = parseFloat(value);
return Number.isNaN(n) ? null : n;
}Then in the formatting paths, check for null and return emptyPlaceholder. This would prevent rendering literal "NaN" in the UI.
| case 'percent': | ||
| return getCurrentD3Locale(locale, currency).format(DEFAULT_PERCENT_FORMAT)(parseNumber(value)); | ||
| case 'number': | ||
| return getCurrentD3Locale(locale, currency).format(DEFAULT_NUMBER_FORMAT)(parseNumber(value)); |
There was a problem hiding this comment.
Nit: The currency parameter is passed to getCurrentD3Locale even for non-currency formats (lines 120-122). It doesn't cause a bug, but it creates unnecessary cache entries when different currency codes are used — e.g., locale:EUR and locale:USD would cache two identical locale objects for format: 'number'.
Consider passing a fixed default for non-currency formats:
| case 'percent': | |
| return getCurrentD3Locale(locale, currency).format(DEFAULT_PERCENT_FORMAT)(parseNumber(value)); | |
| case 'number': | |
| return getCurrentD3Locale(locale, currency).format(DEFAULT_NUMBER_FORMAT)(parseNumber(value)); | |
| case 'percent': | |
| return getCurrentD3Locale(locale).format(DEFAULT_PERCENT_FORMAT)(parseNumber(value)); | |
| case 'number': | |
| return getCurrentD3Locale(locale).format(DEFAULT_NUMBER_FORMAT)(parseNumber(value)); |
| // We don't export it for now, because size of builds for cjs/umd users will be affected | ||
| // export * from './format'; |
There was a problem hiding this comment.
The export is commented out — meaning consumers can't actually use formatValue via the standard @cubejs-client/core import.
Is this intentional for this PR? If so, how are consumers expected to access this function? If this is meant to be usable in a follow-up (e.g., only from ESM builds), it might be worth adding a note in the PR description about the plan.
If tree-shaking is the concern, modern bundlers (Rollup, webpack, esbuild) will eliminate unused exports from ESM builds. The CJS/UMD concern is valid, but you could consider a separate entry point (e.g., @cubejs-client/core/format) instead of blocking the export entirely.
There was a problem hiding this comment.
Claude finished @ovr's task in 2m 22s —— View job
All fixes implemented — push pending
- Fix
parseNumberto returnnullfor NaN (prevents"NaN"in UI) - Fix invalid date handling to return
emptyPlaceholderinstead of garbled output - Fix currency code leaking into cache key for non-currency formats
- Uncomment the
formatValueexport inindex.ts - Add missing test cases (NaN input, invalid date, custom emptyPlaceholder, empty string)
- Push to remote — commit
1dd2fe3is ready locally butgit pushis blocked by the runner's permission mode. Please rungit push origin feat/client-core-formattingto publish.
Changes in commit 1dd2fe3
src/format.ts
parseNumberreturnsnumber | null—nullwhenparseFloatyieldsNaN(e.g."abc","")- All numeric paths (
currency,percent,number,custom-numeric, type-basednumber) returnemptyPlaceholderonnullinstead of formatting literal"NaN" - Both time paths (
custom-time, type-basedtime) validatenew Date(value).getTime()and returnemptyPlaceholderfor invalid dates percent/number/ type-based-number paths no longer passcurrencytogetCurrentD3Locale, eliminating duplicate cache entries
src/index.ts
formatValue,FormatValueOptions,FormatValueMemberare now exported from@cubejs-client/core
test/format.test.ts
non-numeric input returns emptyPlaceholder for numeric formats—"abc"and""with all numeric format pathsinvalid date returns emptyPlaceholder for time formats— type-based andcustom-timecustom emptyPlaceholder— verifies the option threads through to all fallback paths
| expect(formatValue(true, { type: 'boolean' })).toBe('true'); | ||
| expect(formatValue('', { type: 'string' })).toBe(''); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test scenarios to consider:
- NaN / non-numeric input:
formatValue("abc", { type: 'number', format: 'currency' })— currently renders"NaN", which is likely undesirable - Custom
emptyPlaceholder: No test forformatValue(null, { type: 'number', emptyPlaceholder: 'N/A' }) - Invalid date input:
formatValue("not-a-date", { type: 'time' })—new Date("not-a-date")producesInvalid Date - Very large numbers: e.g.,
Number.MAX_SAFE_INTEGERto verify formatting doesn't break - Negative values with non-USD currency: e.g.,
formatValue(-1234.56, { type: 'number', format: 'currency', currency: 'EUR' })
Tests 1 and 3 are the most important since they exercise real-world edge cases where user data might be unexpected.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10653 +/- ##
=======================================
Coverage 57.84% 57.84%
=======================================
Files 215 215
Lines 16609 16609
Branches 3336 3336
=======================================
Hits 9607 9607
Misses 6514 6514
Partials 488 488
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @ovr's task in 2m 47s —— View job PR Review:
|
| Priority | Issue | Location |
|---|---|---|
| 🔴 High | Export is commented out — consumers can't use formatValue. Needs clarity on the access strategy or a note in the PR description. |
index.ts:913-914 |
| 🟡 Medium | parseNumber returns NaN for non-numeric strings — d3 will render literal "NaN" in the UI. Should return null and fall back to emptyPlaceholder. |
format.ts:78-84 |
| 🟡 Medium | new Date() on invalid input produces Invalid Date — timeFormat silently outputs "NaN" placeholders for each directive. Should validate and fall back. |
format.ts:142 |
| 🟢 Low | Currency code leaks into cache key for non-currency formats, causing duplicate cache entries for percent and number formats. |
format.ts:128-130 |
| 🟢 Low | Missing test edge cases: NaN input, invalid dates, custom emptyPlaceholder, empty string with numeric format. |
format.test.ts:84 |
Security
No concerns — this is a pure formatting utility with no network calls, DOM manipulation, or dynamic code execution.
Verdict
The core logic is sound and well-tested for happy paths. The commented-out export is the most important question — it determines whether this feature is actually shippable or a foundation for a follow-up. The NaN/invalid date handling should be addressed before merge to avoid rendering "NaN" in production UIs — these are the most likely edge cases users will hit with real-world data.
| function parseNumber(value: any): number { | ||
| if (value === null || value === undefined) { | ||
| return 0; | ||
| } | ||
|
|
||
| return parseFloat(value); | ||
| } |
There was a problem hiding this comment.
parseNumber silently returns NaN for non-numeric strings, which d3-format will render as literal "NaN" in the UI.
For example, formatValue("hello", { type: 'number', format: 'currency' }) will output "NaN". This is likely undesirable in a user-facing formatting utility. Consider returning null and falling back to emptyPlaceholder:
| function parseNumber(value: any): number { | |
| if (value === null || value === undefined) { | |
| return 0; | |
| } | |
| return parseFloat(value); | |
| } | |
| function parseNumber(value: any): number | null { | |
| if (value === null || value === undefined) { | |
| return null; | |
| } | |
| const n = parseFloat(value); | |
| return Number.isNaN(n) ? null : n; | |
| } |
Then in formatValue, check for null before formatting:
case 'currency': {
const n = parseNumber(value);
return n === null ? emptyPlaceholder : getCurrentD3Locale(locale, currency).format(DEFAULT_CURRENCY_FORMAT)(n);
}This applies to all numeric formatting paths (currency, percent, number, custom-numeric, and the type-based number fallback).
| return getCurrentD3Locale(locale, currency).format(DEFAULT_PERCENT_FORMAT)(parseNumber(value)); | ||
| case 'number': | ||
| return getCurrentD3Locale(locale, currency).format(DEFAULT_NUMBER_FORMAT)(parseNumber(value)); |
There was a problem hiding this comment.
Minor: currency parameter leaks into cache key for non-currency formats.
getCurrentD3Locale(locale, currency) is called for percent and number formats too, but the currency value only affects the currency symbol in the locale definition. This means getCurrentD3Locale('en-US', 'EUR').format(',.2~f') and getCurrentD3Locale('en-US', 'USD').format(',.2~f') produce identical results but occupy different cache slots.
Consider passing currency only for the 'currency' case:
| return getCurrentD3Locale(locale, currency).format(DEFAULT_PERCENT_FORMAT)(parseNumber(value)); | |
| case 'number': | |
| return getCurrentD3Locale(locale, currency).format(DEFAULT_NUMBER_FORMAT)(parseNumber(value)); | |
| case 'percent': | |
| return getCurrentD3Locale(locale).format(DEFAULT_PERCENT_FORMAT)(parseNumber(value)); | |
| case 'number': | |
| return getCurrentD3Locale(locale).format(DEFAULT_NUMBER_FORMAT)(parseNumber(value)); |
Same applies to the type-based number fallback on line 146.
| // No explicit format — infer from type | ||
| if (type === 'time') { | ||
| const fmt = getTimeFormatByGrain(granularity); | ||
| return timeFormat(fmt)(new Date(value)); |
There was a problem hiding this comment.
new Date(value) on invalid input produces Invalid Date, and timeFormat will silently output "NaN" placeholders for each directive.
For example, formatValue("not-a-date", { type: 'time' }) renders something like "NaN-NaN-NaN NaN:NaN:NaN". Consider validating the date:
const d = new Date(value);
if (Number.isNaN(d.getTime())) {
return emptyPlaceholder;
}
return timeFormat(fmt)(d);This also applies to the custom-time path on line 116.
| // We don't export it for now, because size of builds for cjs/umd users will be affected | ||
| // export * from './format'; |
There was a problem hiding this comment.
The export is still commented out — consumers can't access formatValue via the standard @cubejs-client/core import.
Is this intentional for this PR? If the concern is bundle size for CJS/UMD users, a separate entry point (e.g., @cubejs-client/core/format) could expose it without affecting existing builds. Alternatively, modern bundlers tree-shake unused ESM exports effectively.
If this is meant to land without the export, it might be worth noting in the PR description that this is a foundational PR with a follow-up planned to wire the export.
| expect(formatValue(true, { type: 'boolean' })).toBe('true'); | ||
| expect(formatValue('', { type: 'string' })).toBe(''); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Consider adding tests for edge cases that real-world data will hit:
- Non-numeric input with numeric format:
formatValue("abc", { type: 'number', format: 'currency' })— currently renders"NaN" - Invalid date input:
formatValue("not-a-date", { type: 'time' })— renders garbled"NaN-NaN-NaN..." - Custom
emptyPlaceholder:formatValue(null, { type: 'number', emptyPlaceholder: 'N/A' })— should return'N/A' - Empty string with numeric format:
formatValue("", { type: 'number', format: 'number' })—parseFloat("")isNaN
Tests 1, 2, and 4 are the most important since they exercise cases where user data might be unexpected and the current behavior produces undesirable output.
2 participants
No description provided.