Copilot commented on code in PR #39156:
URL: https://github.com/apache/superset/pull/39156#discussion_r3223709671
##########
superset-frontend/plugins/preset-chart-deckgl/src/utils/safeStringify.ts:
##########
@@ -39,14 +35,14 @@ export function safeStringify(object: JsonObject) {
// Quick deep copy to duplicate if this is a repeat rather than a
circle.
return JSON.parse(JSON.stringify(value));
} catch (error) {
- // Discard key if value cannot be duplicated.
- return;
+ // Replace circular reference with a placeholder
+ console.warn(`Circular reference detected and replaced with
'[Circular]' placeholder (key: "${key}")`);
+ return '[Circular]';
Review Comment:
The PR description says the duplicated implementation in the Deck.gl plugin
was removed and re-exported from core, but this diff still contains a full
local implementation. Either update the plugin to re-export/import the shared
implementation (as described), or adjust the PR description to match what’s
actually being shipped.
##########
superset-frontend/src/utils/safeStringify.test.ts:
##########
@@ -49,17 +49,12 @@ describe('Stringify utility testing', () => {
test('handles simple circular json as expected', () => {
const ping = new Noise();
const pong = new Noise();
- const pang = new Noise();
ping.next = pong;
pong.next = ping;
- // ping.next is pong (the circular reference) now
+ // It should now safely output [Circular] for the recursive reference
const safeString = safeStringify(ping);
- ping.next = pang;
-
- // ping.next is pang now, which has no circular reference, so it's safe to
use JSON.stringify
- const ordinaryString = JSON.stringify(ping);
- expect(safeString).toEqual(ordinaryString);
+ expect(safeString).toEqual('{"next":{"next":"[Circular]"}}');
Review Comment:
This test triggers the new `console.warn` path (circular reference), which
can clutter test output or fail in setups that disallow console warnings.
Consider explicitly stubbing/spying `console.warn` within the test and
asserting it was called (or silencing it) to keep test runs clean and
deterministic.
##########
superset-frontend/src/utils/safeStringify.ts:
##########
@@ -33,8 +32,11 @@ export function safeStringify(object: any): string {
// Quick deep copy to duplicate if this is a repeat rather than a
circle.
return JSON.parse(JSON.stringify(value));
} catch (err) {
- // Discard key if value cannot be duplicated.
- return; // eslint-disable-line consistent-return
+ // Replace circular reference with a placeholder
+ console.warn(
+ `Circular reference detected and replaced with '[Circular]'
placeholder (key: "${key}")`,
+ );
+ return '[Circular]';
Review Comment:
Emitting `console.warn` inside a general-purpose stringify utility can
create very noisy logs in production (potentially once per circular occurrence
per call) and adds a side-effect to a utility that otherwise returns a value.
Consider making logging opt-in (e.g., via an options argument or injected
logger), gating it to non-production builds, or warning at most once per
`safeStringify` invocation.
##########
superset-frontend/plugins/preset-chart-deckgl/src/utils/safeStringify.ts:
##########
@@ -39,14 +35,14 @@ export function safeStringify(object: JsonObject) {
// Quick deep copy to duplicate if this is a repeat rather than a
circle.
return JSON.parse(JSON.stringify(value));
} catch (error) {
- // Discard key if value cannot be duplicated.
- return;
+ // Replace circular reference with a placeholder
+ console.warn(`Circular reference detected and replaced with
'[Circular]' placeholder (key: "${key}")`);
Review Comment:
This `console.warn` line is significantly longer than the surrounding style
used in the other `safeStringify` implementation (which formats the warning
across multiple lines). Reformatting consistently helps readability and reduces
the chance of failing any line-length formatting/lint rules used in the repo.
##########
superset-frontend/src/utils/safeStringify.test.ts:
##########
@@ -68,9 +63,9 @@ describe('Stringify utility testing', () => {
ping.next = pong;
pong.next = ping;
- const newNoise: Noise = JSON.parse(safeStringify(ping));
+ const newNoise: any = JSON.parse(safeStringify(ping));
Review Comment:
Switching the parsed result type from `Noise` to `any` weakens the test’s
type guarantees. Prefer `unknown` with a narrow type assertion for the specific
shape you assert on (e.g., `{ next: { next: string } }`) so the test stays
type-safe while still reflecting that JSON.parse returns untyped data.
--
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]
