Open
Conversation
9 tasks
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #9595bb
Actionable Suggestions - 1
-
superset/static/service-worker.js - 1
- Missing break statement in switch case · Line 536-536
Additional Suggestions - 1
-
superset/static/service-worker.js - 1
-
Typo in error message · Line 1116-1116The error message in the default case of the HMR switch statement has a typo: 'Unexception type' should be 'Unexpected type'. This is generated webpack code, but fixing the typo improves error message clarity.
Code suggestion
@@ -1116,1 +1116,1 @@ - /******/ throw new Error("Unexception type " + result.type); + /******/ throw new Error("Unexpected type " + result.type);
-
Review Details
-
Files reviewed - 4 · Commit Range:
f7ca733..88d258d- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
- superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.test.ts
- superset/static/service-worker.js
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
superset/static/service-worker.js
Outdated
| /******/ case "ready": | ||
| /******/ setStatus("prepare"); | ||
| /******/ /* fallthrough */ | ||
| /******/ case "prepare": |
Contributor
There was a problem hiding this comment.
Missing break statement in switch case
Switch case at line 536 is missing a break statement before the next case, causing fall-through behavior. This is in webpack's HMR code; verify if fall-through is intentional or add break statement.
Code suggestion
Check the AI-generated fix before applying
@@ -535,3 +535,4 @@
case "prepare":
blockingPromises++;
promise.then(unblock, unblock);
+ break;
return promise;
Code Review Run #9595bb
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
msyavuz
changed the title
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
superset/static/service-worker.js
Outdated
Comment on lines
1464
to
1468
| /******/ __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./node_modules/webpack-dev-server/client/index.js?protocol=ws%3A&hostname=0.0.0.0&port=0&pathname=%2Fws&logging=info&overlay=%7B%22errors%22%3Atrue%2C%22warnings%22%3Afalse%2C%22runtimeErrors%22%3A%22error%2520%253D%253E%2520%21%252FResizeObserver%252F.test%28error.message%29%22%7D&reconnect=10&hot=only&live-reload=false"))) | ||
| /******/ __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./node_modules/webpack/hot/only-dev-server.js"))) | ||
| /******/ __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./node_modules/@pmmmwh/react-refresh-webpack-plugin/client/ErrorOverlayEntry.js"))) | ||
| /******/ var __webpack_exports__ = __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./src/service-worker.ts"))) | ||
| /******/ __webpack_exports__ = __webpack_require__.O(__webpack_exports__); |
Contributor
There was a problem hiding this comment.
Suggestion: The service worker bundle is executing Webpack dev-server and React Refresh client entrypoints, which assume a window/document environment and will throw runtime errors in the service worker context (which has no DOM), preventing the worker from installing; this can be fixed by only loading the actual service worker module as the entrypoint and not the dev-only clients. [logic error]
Severity Level: Critical 🚨
- ❌ Service worker registration fails on SPA load.
- ⚠️ PWA manifest and offline handling never activate.
- ⚠️ Browser console logs errors from dev-only runtime.
Suggested change
| /******/ __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./node_modules/webpack-dev-server/client/index.js?protocol=ws%3A&hostname=0.0.0.0&port=0&pathname=%2Fws&logging=info&overlay=%7B%22errors%22%3Atrue%2C%22warnings%22%3Afalse%2C%22runtimeErrors%22%3A%22error%2520%253D%253E%2520%21%252FResizeObserver%252F.test%28error.message%29%22%7D&reconnect=10&hot=only&live-reload=false"))) | |
| /******/ __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./node_modules/webpack/hot/only-dev-server.js"))) | |
| /******/ __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./node_modules/@pmmmwh/react-refresh-webpack-plugin/client/ErrorOverlayEntry.js"))) | |
| /******/ var __webpack_exports__ = __webpack_require__.O(undefined, ["vendors","vendors-node_modules_rc-component_color-picker_es_index_js-node_modules_rc-component_mutate-o-484854","webpack_sharing_consume_default_react-dom_react-dom-webpack_sharing_consume_default_react_rea-153fef"], () => (__webpack_require__("./src/service-worker.ts"))) | |
| /******/ __webpack_exports__ = __webpack_require__.O(__webpack_exports__); | |
| /******/ var __webpack_exports__ = __webpack_require__("./src/service-worker.ts"); |
Steps of Reproduction ✅
1. Build and run the frontend in development mode so Webpack dev-server is used with
`webpack.config.js` (see `superset-frontend/webpack.config.js:55-69` for `mode` and
`isDevMode`/`isDevServer` flags).
2. Note that the Webpack entry config at `superset-frontend/webpack.config.js:312-321`
defines a `service-worker` entry pointing at `src/service-worker.ts`, and the output
config at `webpack.config.js:86-96` writes that bundle to `../service-worker.js` (i.e.
`superset/static/service-worker.js`).
3. Open the SPA in a browser so `superset/templates/superset/spa.html` is rendered; the
inline script at `spa.html:75-82` checks `'serviceWorker' in navigator` and calls
`navigator.serviceWorker.register('{{ assets_prefix }}/static/service-worker.js')`,
causing the browser to fetch and execute `superset/static/service-worker.js`.
4. When `superset/static/service-worker.js` executes, the bottom of the bundle (lines
1460-1471 in the PR diff) runs `__webpack_require__` for dev-only modules
`./node_modules/@pmmmwh/react-refresh-webpack-plugin/client/ReactRefreshEntry.js`,
`./node_modules/webpack-dev-server/client/index.js`,
`./node_modules/webpack/hot/only-dev-server.js`, and the React Refresh error overlay
client before finally requiring `"./src/service-worker.ts"`.
5. Those dev-only clients assume a window/document DOM environment (e.g. Webpack
dev-server client and the React Refresh error overlay manipulate `document` to render
overlays), but the service worker global scope has no `document`; when these modules are
required as part of the service worker script, they throw at module top level (e.g.
`ReferenceError: document is not defined`), aborting evaluation of `service-worker.js`
before the `install`/`activate` handlers from
`superset-frontend/src/service-worker.ts:30-36` can be registered.
6. As a result, the call to `navigator.serviceWorker.register` in `spa.html:75-82` fails
(visible in DevTools console / Application → Service Workers as an error on
`static/service-worker.js`), and the service worker never reaches the `install` or
`activate` events, disabling the intended PWA/file-handling behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/static/service-worker.js
**Line:** 1464:1468
**Comment:**
*Logic Error: The service worker bundle is executing Webpack dev-server and React Refresh client entrypoints, which assume a window/document environment and will throw runtime errors in the service worker context (which has no DOM), preventing the worker from installing; this can be fixed by only loading the actual service worker module as the entrypoint and not the dev-only clients.
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.
Contributor
Code Review Agent Run #95f8a0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
bot
added
🎪 532674a 🚦 building
Contributor
Comment on lines
+99
to
+107
| } else if ( | ||
| timeGrain === TimeGranularity.HOUR || | ||
| timeGrain === TimeGranularity.THIRTY_MINUTES || | ||
| timeGrain === TimeGranularity.FIFTEEN_MINUTES || | ||
| timeGrain === TimeGranularity.TEN_MINUTES || | ||
| timeGrain === TimeGranularity.FIVE_MINUTES || | ||
| timeGrain === TimeGranularity.MINUTE || | ||
| timeGrain === TimeGranularity.SECOND | ||
| ) { |
Contributor
There was a problem hiding this comment.
Suggestion: Grouping all sub-hour time grains (e.g., 30/15/5 minutes, seconds) into the same branch as the hourly grain and normalizing them to the top of the hour zeros out their minute/second components, so distinct time buckets within the same hour produce identical formatted labels and misrepresent the actual bucket boundaries. Restrict this normalization to the pure hourly grain so that finer-grained buckets preserve their minute/second resolution while still having milliseconds stripped earlier in the function. [logic error]
Severity Level: Major ⚠️
- ⚠️ Timeseries chart x-axis mislabels sub-hour time buckets.
- ⚠️ Users may misinterpret 30/15/5-minute trend boundaries.
- ⚠️ Time-based analyses lose clarity at finer granularity.
Suggested change
| } else if ( | |
| timeGrain === TimeGranularity.HOUR || | |
| timeGrain === TimeGranularity.THIRTY_MINUTES || | |
| timeGrain === TimeGranularity.FIFTEEN_MINUTES || | |
| timeGrain === TimeGranularity.TEN_MINUTES || | |
| timeGrain === TimeGranularity.FIVE_MINUTES || | |
| timeGrain === TimeGranularity.MINUTE || | |
| timeGrain === TimeGranularity.SECOND | |
| ) { | |
| } else if (timeGrain === TimeGranularity.HOUR) { |
Steps of Reproduction ✅
1. In Superset, create or open a Timeseries ECharts chart (handled by
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:117-211`),
using a temporal column on the x-axis so `xAxisDataType === GenericDataType.Temporal` (see
lines 250-252).
2. In the chart controls, set **Time Grain** to a sub-hour grain such as `30 minutes` so
that `timeGrainSqla` is a sub-hour `TimeGranularity` value like
`TimeGranularity.THIRTY_MINUTES` (`transformProps.ts:181`), and set **X-Axis Time Format**
to "Adaptive formatting" (maps to `SMART_DATE_ID`, passed as `xAxisTimeFormat` at
`transformProps.ts:186,199`).
3. On render, `transformProps` computes the x-axis formatter for temporal data as
`getXAxisFormatter(xAxisTimeFormat, timeGrainSqla)` at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:559-566`.
With `xAxisTimeFormat = SMART_DATE_ID`, `getXAxisFormatter` returns
`getSmartDateFormatter(timeGrainSqla)` at
`superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:172-178`.
4. Inside `getSmartDateFormatter`'s `formatFunc` (`formatters.ts:51-118`), when
`timeGrainSqla` is any of `TimeGranularity.THIRTY_MINUTES`, `FIFTEEN_MINUTES`,
`TEN_MINUTES`, `FIVE_MINUTES`, `MINUTE`, or `SECOND`, the current branch at
`formatters.ts:99-115` executes, normalizing each bucket's `Date` to the *top of the hour*
(minutes and seconds set to `0`) before passing it to the base SMART_DATE formatter. As a
result, distinct 30-minute (or finer) buckets within the same hour (e.g., `10:00` and
`10:30`) produce identical axis labels like `"2025-03-15 10:00"`, causing
duplicate/misleading x-axis labels in the rendered ECharts Timeseries chart.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
**Line:** 99:107
**Comment:**
*Logic Error: Grouping all sub-hour time grains (e.g., 30/15/5 minutes, seconds) into the same branch as the hourly grain and normalizing them to the top of the hour zeros out their minute/second components, so distinct time buckets within the same hour produce identical formatted labels and misrepresent the actual bucket boundaries. Restrict this normalization to the pure hourly grain so that finer-grained buckets preserve their minute/second resolution while still having milliseconds stripped earlier in the 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.
github-actions
bot
added
🎪 532674a 🚦 deploying
Contributor
|
🎪 Showtime deployed environment on GHA for 532674a • Environment: http://35.161.243.247:8080 (admin/admin) |
Contributor
Code Review Agent Run #b1c7c4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
bot
removed
🎪 532674a 🤡 msyavuz
Contributor
Sequence DiagramThe PR ensures transformProps passes the chart's time grain to the X-axis formatter and replaces the undefined adaptive formatter with a time-grain-aware smart date formatter so ECharts renders human-friendly date labels instead of raw timestamps. sequenceDiagram
participant transformProps
participant FormattersUtils
participant TimeFormatter
participant ECharts
transformProps->>FormattersUtils: getXAxisFormatter(xAxisTimeFormat, timeGrainSqla)
FormattersUtils->>TimeFormatter: create smart-date formatter (time-grain-aware)
TimeFormatter-->>FormattersUtils: return TimeFormatter (normalizes dates by grain)
FormattersUtils-->>transformProps: TimeFormatter
transformProps->>ECharts: attach axisLabel.formatter = TimeFormatter
ECharts->>TimeFormatter: format(date) when rendering labels
TimeFormatter-->>ECharts: formatted date string
Generated by CodeAnt AI |
Contributor
Code Review Agent Run #7e2479Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
This PR fixes a critical bug in ECharts Bar Chart X-axis labels where "Adaptive Formatting" was displaying raw timestamps with milliseconds (e.g., "21:13:32 389") instead of properly
formatted dates like "Jan 2025" or "2004".
Root Cause: The getXAxisFormatter function was returning undefined for adaptive formatting, causing ECharts to fall back to its default formatter which didn't handle dates properly.
Solution: Enhanced the smart date formatter to be time grain-aware, ensuring consistent date formatting based on the chart's time granularity (year, quarter, month, week, day, hour).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION