codeant-ai-for-open-source[bot] commented on code in PR #36702: URL: https://github.com/apache/superset/pull/36702#discussion_r2626294979
########## superset-frontend/packages/superset-ui-core/test/time-format/formatters/relativeDayFormatter.test.ts: ########## @@ -0,0 +1,74 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { createRelativeDayFormatter } from '@superset-ui/core'; + +test('createRelativeDayFormatter returns a formatter', () => { + const formatter = createRelativeDayFormatter(); + expect(formatter).toBeDefined(); + expect(typeof formatter.format).toBe('function'); Review Comment: **Suggestion:** Incorrect type assertion: the test checks `typeof formatter.format` but `createRelativeDayFormatter()` returns a callable formatter (a function) rather than an object with a `.format` method; this causes the test to fail because `formatter.format` is undefined. Change the assertion to verify that `formatter` itself is a function. [type error] **Severity Level:** Minor ⚠️ ```suggestion expect(typeof formatter).toBe('function'); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The tests call formatter(date) elsewhere in the file, so createRelativeDayFormatter returns a callable. Checking typeof formatter.format is incorrect (formatter.format will be undefined) and will make the test fail. Replacing the assertion with typeof formatter === 'function' directly fixes a real test bug. </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/time-format/formatters/relativeDayFormatter.test.ts **Line:** 25:25 **Comment:** *Type Error: Incorrect type assertion: the test checks `typeof formatter.format` but `createRelativeDayFormatter()` returns a callable formatter (a function) rather than an object with a `.format` method; this causes the test to fail because `formatter.format` is undefined. Change the assertion to verify that `formatter` itself is a function. 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/plugins/plugin-chart-echarts/src/utils/formatters.ts: ########## @@ -92,6 +94,12 @@ export function getXAxisFormatter( if (format === SMART_DATE_ID || !format) { return undefined; } + if (format === RELATIVE_DAY_ID) { + return getTimeFormatter(RELATIVE_DAY_ID); + } + if (format === RELATIVE_DAY_NO_TIME_ID) { + return getTimeFormatter(RELATIVE_DAY_NO_TIME_ID); Review Comment: **Suggestion:** Duplicate conditional branches for the two relative-day formats increase maintenance burden and risk of inconsistent behaviour; combine them into a single conditional and call `getTimeFormatter` with the actual `format` value (with a string type guard) to avoid repeating nearly identical code and accidental drift between branches. [logic error] **Severity Level:** Minor ⚠️ ```suggestion if (typeof format === 'string' && (format === RELATIVE_DAY_ID || format === RELATIVE_DAY_NO_TIME_ID)) { return getTimeFormatter(format); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The proposed consolidation removes duplicated branches without changing runtime behavior — when format equals one of those constants, calling getTimeFormatter(format) is equivalent to passing the constant. It's a small, safe maintenance improvement (the typeof guard is redundant given the signature but harmless). </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts **Line:** 97:101 **Comment:** *Logic Error: Duplicate conditional branches for the two relative-day formats increase maintenance burden and risk of inconsistent behaviour; combine them into a single conditional and call `getTimeFormatter` with the actual `format` value (with a string type guard) to avoid repeating nearly identical code and accidental drift between branches. 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/time-format/formatters/relativeDayFormatterNoTime.ts: ########## @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import TimeFormatter from '../TimeFormatter'; + +export const RELATIVE_DAY_NO_TIME_ID = 'relative_day_no_time'; + +// Reference date: January 1, 2000 00:00:00 UTC = Day 1 +const DAY_ONE_DATE = Date.UTC(2000, 0, 1, 0, 0, 0, 0); +const MS_PER_DAY = 86400000; + +export function createRelativeDayFormatterNoTime() { + return new TimeFormatter({ + id: RELATIVE_DAY_NO_TIME_ID, + label: 'Relative Day (No Time)', + description: 'Formats dates as relative days from Day 1 (01/01/2000) without time', + formatFunc: (date: Date) => { + const ms = date.getTime(); + Review Comment: **Suggestion:** Runtime failure for invalid dates: calling `getTime()` on an invalid `Date` returns NaN which will propagate and produce `Day NaN`; detect invalid dates (Number.isNaN on milliseconds) and return a sensible string (e.g., 'Invalid Date') to avoid confusing output or downstream errors. [possible bug] **Severity Level:** Critical 🚨 ```suggestion if (Number.isNaN(ms)) { return 'Invalid Date'; } ``` <details> <summary><b>Why it matters? ⭐ </b></summary> Valid point — an invalid Date yields NaN and the formatter would return "Day NaN", which is confusing. Guarding against Number.isNaN(ms) and returning a sensible fallback (or empty string) prevents misleading output. </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/time-format/formatters/relativeDayFormatterNoTime.ts **Line:** 35:35 **Comment:** *Possible Bug: Runtime failure for invalid dates: calling `getTime()` on an invalid `Date` returns NaN which will propagate and produce `Day NaN`; detect invalid dates (Number.isNaN on milliseconds) and return a sensible string (e.g., 'Invalid Date') to avoid confusing output or downstream errors. 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]
