fix(echarts): adaptive formatting labels#38017
Conversation
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
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": |
There was a problem hiding this comment.
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
| /******/ __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__); |
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.| /******/ __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.
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 |
|
🎪 Showtime deployed environment on GHA for 532674a • Environment: http://35.161.243.247:8080 (admin/admin) |
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 |
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 |
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 |
sadpandajoe
added
the
🎪 ⚡ showtime-trigger-start
github-actions
bot
added
🎪 430ded9 🚦 building
and removed
🎪 ⚡ showtime-trigger-start
|
🎪 Showtime deployed environment on GHA for 430ded9 • Environment: http://35.166.2.103:8080 (admin/admin) |
|
🎪 Showtime deployed environment on GHA for 430ded9 • Environment: http://54.200.206.12:8080 (admin/admin) |
|
The testing for this ticket was completed. The application is working as expected. ScenarioVerify the user should not see a timestamp displayed at the end of the chart X Axis when the user creates/edits a bar chart using the time grain month.
Steps
Expected resultAfter the step 5, the user should not see a timestamp displayed at the end of the chart X Axis. Current resultAfter the step 5, the user is not seeing a timestamp displayed at the end of the chart X Axis. EvidenceBefore - Master current behavior - old behavior
After - Fixed Bar chart without time stamp at the end - quarter
After - Fixed Bar chart without time stamp at the end - month
After - Fixed Line chart without time stamp at the end - month
|
6 participants
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