codeant-ai-for-open-source[bot] commented on code in PR #37039:
URL: https://github.com/apache/superset/pull/37039#discussion_r2680834046
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -60,7 +60,11 @@ class CurrencyFormatter extends ExtensibleFunction {
}
getNormalizedD3Format() {
- return this.d3Format.replace(/\$|%/g, '');
+ return this.d3Format.replaceAll('$', '');
Review Comment:
**Suggestion:** Compatibility/robustness: `getNormalizedD3Format` uses
`replaceAll('$', '')`, which may not be available in all JS runtimes; using a
global-regex `replace(/\$/g, '')` is equivalent and more compatible across
environments. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
return this.d3Format.replace(/\$/g, '');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using replace(/\\$/g, '') is more compatible across older JS engines than
replaceAll. The improved code is correct and safer for environments without
String.prototype.replaceAll. Note: the file also uses replaceAll in
normalizeForCurrency — to be consistent you should change both or rely on a
polyfill/target that supports replaceAll.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts
**Line:** 63:63
**Comment:**
*Possible Bug: Compatibility/robustness: `getNormalizedD3Format` uses
`replaceAll('$', '')`, which may not be available in all JS runtimes; using a
global-regex `replace(/\$/g, '')` is equivalent and more compatible across
environments.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/packages/superset-ui-core/test/currency-format/CurrencyFormatter.test.ts:
##########
@@ -146,9 +148,9 @@ test('CurrencyFormatter:format', () => {
const currencyFormatterWithPercentD3 = new CurrencyFormatter({
currency: { symbol: 'USD', symbolPosition: 'prefix' },
- d3Format: ',.1f%',
+ d3Format: ',.1%',
});
- expect(currencyFormatterWithPercentD3(VALUE)).toEqual('$ 56,100,057.0');
+ expect(currencyFormatterWithPercentD3(VALUE)).toEqual('$ 5,610,005,700.0');
Review Comment:
**Suggestion:** The test's expected string for percent formatting is missing
the percent sign produced by the d3 percent specifier; this will cause the
assertion to fail because d3-format multiplies the value by 100 and appends '%'
when using '%' formatting. Update the expectation to include the '%' suffix.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
expect(currencyFormatterWithPercentD3(VALUE)).toEqual('$
5,610,005,700.0%');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This test expectation is incorrect: d3's '%' specifier multiplies the value
by 100 and appends '%' to the formatted output.
For VALUE=56100057 the formatted number becomes "5,610,005,700.0%" and the
currency wrapper adds the "$ " prefix, so the correct expectation is "$
5,610,005,700.0%".
Fixing this addresses a real test failure rather than a stylistic issue.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/test/currency-format/CurrencyFormatter.test.ts
**Line:** 153:153
**Comment:**
*Logic Error: The test's expected string for percent formatting is
missing the percent sign produced by the d3 percent specifier; this will cause
the assertion to fail because d3-format multiplies the value by 100 and appends
'%' when using '%' formatting. Update the expectation to include the '%' suffix.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -71,10 +75,11 @@ class CurrencyFormatter extends ExtensibleFunction {
return formattedValue as string;
}
+ const normalizedValue = this.normalizeForCurrency(formattedValue);
if (this.currency.symbolPosition === 'prefix') {
- return `${getCurrencySymbol(this.currency)} ${formattedValue}`;
+ return `${getCurrencySymbol(this.currency)} ${normalizedValue}`;
}
- return `${formattedValue} ${getCurrencySymbol(this.currency)}`;
+ return `${normalizedValue} ${getCurrencySymbol(this.currency)}`;
Review Comment:
**Suggestion:** Runtime/logic bug: `getCurrencySymbol(this.currency)` can
return `undefined` or throw (Intl.NumberFormat will throw if given an invalid
currency code), and the code interpolates its result directly into template
strings producing "undefined" or crashing at runtime. Capture the symbol once,
fall back to `this.currency.symbol` (or empty string), and guard the Intl call
with try/catch to avoid crashes. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const currencySymbol = (() => {
try {
return getCurrencySymbol(this.currency) ?? this.currency?.symbol ??
'';
} catch {
return this.currency?.symbol ?? '';
}
})();
if (this.currency.symbolPosition === 'prefix') {
return `${currencySymbol} ${normalizedValue}`;
}
return `${normalizedValue} ${currencySymbol}`;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Reasonable and actionable. hasValidCurrency only checks truthiness of
this.currency?.symbol — it doesn't guarantee Intl.NumberFormat will accept the
code or that getCurrencySymbol won't return undefined. Caching the symbol,
catching Intl errors and falling back to this.currency.symbol (or empty string)
prevents a runtime throw or "undefined" in output. This change improves
robustness.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts
**Line:** 79:82
**Comment:**
*Possible Bug: Runtime/logic bug: `getCurrencySymbol(this.currency)`
can return `undefined` or throw (Intl.NumberFormat will throw if given an
invalid currency code), and the code interpolates its result directly into
template strings producing "undefined" or crashing at runtime. Capture the
symbol once, fall back to `this.currency.symbol` (or empty string), and guard
the Intl call with try/catch to avoid crashes.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/packages/superset-ui-core/test/currency-format/CurrencyFormatter.test.ts:
##########
@@ -109,7 +109,9 @@ test('CurrencyFormatter:getNormalizedD3Format', () => {
currency: { symbol: 'USD', symbolPosition: 'prefix' },
d3Format: ',.1%',
});
- expect(currencyFormatter4.getNormalizedD3Format()).toEqual(',.1');
+ expect(currencyFormatter4.getNormalizedD3Format()).toEqual(
+ currencyFormatter4.d3Format,
+ );
Review Comment:
**Suggestion:** The new test compares `getNormalizedD3Format()` against the
instance property `d3Format`; using the property can make the test fragile if
the property is mutated elsewhere. Assert the explicit expected normalized
format string (',.1%') instead of referencing the property to make the test
deterministic. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(currencyFormatter4.getNormalizedD3Format()).toEqual(',.1%');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Asserting against an explicit expected string (',.1%') makes the test
clearer and less coupled to the instance's internal property, improving
determinism.
The current check passes today but is slightly less strict; this change is a
safe improvement to the test's intent.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/test/currency-format/CurrencyFormatter.test.ts
**Line:** 112:114
**Comment:**
*Possible Bug: The new test compares `getNormalizedD3Format()` against
the instance property `d3Format`; using the property can make the test fragile
if the property is mutated elsewhere. Assert the explicit expected normalized
format string (',.1%') instead of referencing the property to make the test
deterministic.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
