codeant-ai-for-open-source[bot] commented on PR #36818: URL: https://github.com/apache/superset/pull/36818#issuecomment-3687671106
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-156a19f1ff3132a881eab6f7f8eab6a9fbbfb77fa1fb9d6e7166e40e5a1f4aa3R63-R66'><strong>XSS Risk</strong></a><br>The function `formatValueAsPill` returns raw string values when they contain '<' or '<' instead of escaping or sanitizing them. Returning unescaped HTML can lead to cross-site scripting if the data is untrusted or later injected into the DOM as HTML. This must be validated and sanitized before being returned or clearly documented and enforced by the rendering layer.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-e2a0da4878b8c9c245c41851bd45ad880b554d4be41757bccb4b1cf35d3b6b43R119-R129'><strong>Division-by-zero edge case</strong></a><br>The percent-change calculation treats falsy `compareValue` (e.g., 0) the same as `null` and avoids division by zero by using a truthiness check. This masks legitimate comparisons against zero and can yield incorrect percent-change results. Use an explicit zero check (Math.abs(compareValue) > 0) or handle zero separately.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-e2a0da4878b8c9c245c41851bd45ad880b554d4be41757bccb4b1cf35d3b6b43R172-R183'><strong>Falsy timestamp checks</strong></a><br>Code uses truthiness checks (e.g., `!trendLineData[0][0]`) to detect missing timestamps. A timestamp value of 0 (Unix epoch) will be treated as missing. Use explicit null/undefined checks (e.g., `== null`) to avoid incorrectly treating valid timestamps as absent.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-e2a0da4878b8c9c245c41851bd45ad880b554d4be41757bccb4b1cf35d3b6b43R105-R110'><strong>Sort comparator handling nulls</strong></a><br>The sort comparator returns 0 when either timestamp is null, preserving original order. This can leave null timestamps mixed in the middle of the series. Consider explicitly ordering null timestamps to the start or end so downstream logic (taking sortedData[0]) is deterministic.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-e977ec67327a46837e80d71c939d33e9fba4f1a1515ab6a9be86fa71f75868ffR142-R145'><strong>Mutable global controls</strong></a><br>The formDataOverrides uses getStandardizedControls().shiftMetric() which internally calls Array.shift(), mutating the controls array. This can introduce cross-component side effects if getStandardizedControls() returns a shared object. Prefer a non-mutating accessor to avoid surprising state changes.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-601b2dc4d11243655e8785fca03693044a2b93f748c81ab4ba2cd449cbca9d9bR96-R125'><strong>Merge ordering bug</strong></a><br>Defaults and user options are merged in the wrong order in some places (defaults may overwrite user-provided values). This leads to user options being unexpectedly replaced by defaults instead of filling missing fields.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-601b2dc4d11243655e8785fca03693044a2b93f748c81ab4ba2cd449cbca9d9bR112-R136'><strong>Null handling / typeof check</strong></a><br>Many object checks use `typeof ... === 'object'` but don't guard against `null`. `null` will pass the typeof check and may produce incorrect merges or runtime issues. Use explicit null checks.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-afc7bca81838b3f020cc8329281f93dd6af0d4bb7d662eef3cd27657d6e8e0e4R73-R78'><strong>Merge semantics for arrays</strong></a><br>The code uses `lodash.merge` to apply `userOverrides`. `lodash.merge` performs deep merging that merges arrays by index, which often leads to surprising results (partial array merges) when users expect arrays to be replaced by overrides. This can create unexpected ECharts option shapes. Consider replacing or customizing the merge behavior so arrays are replaced rather than merged element-by-element.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-5910d96c0f791d6faa726b25922f3f8496f710cdfdb09d2018bbf852e58db638R53-R56'><strong>Unsafe assumptions about theme API</strong></a><br>The function calls `PTM_THEME.echarts.dataZoom.create(...)` and assumes it returns an object with `.slider` and `.inside`. If the theme or create function is missing or returns a different shape, this will throw at runtime. Add runtime guards and fallbacks so charts don't break when the theme shape differs or is undefined.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-5910d96c0f791d6faa726b25922f3f8496f710cdfdb09d2018bbf852e58db638R50-R52'><strong>Inset parsing</strong></a><br>The code converts `ptmZoomInset` to a Number using `Number(insetRaw ?? 24)`, which will produce NaN for non-numeric strings (e.g., 'auto', '') and then propagate that NaN into theme creation. This can cause incorrect positioning/behavior of dataZoom or runtime issues. Validate/normalize and clamp the inset value and provide a safe default when parsing fails.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-1349d1df466ac2f93e7b5db79a83ab33c7e9dacab5e1649c66b332cb140e6530R15-R19'><strong>Overwriting default plugins</strong></a><br>The code assigns a new array to `config.plugins`, which will replace any plugins that `getConfig()` already provided. This can remove necessary default plugins/presets from the Airbnb config and lead to unexpected build/runtime behavior. Prefer merging or appending to the existing `config.plugins`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-5c8145f33327aca41a9ccc92a955d24a64e4fe7901620cc7c57517e4f60a2879R83-R116'><strong>Dependency / key management</strong></a><br>The code imports 'plugins/plugin-chart-flask' and '@superset-ui/superset-plugin-chart-echarts-ptm' — ensure these packages are added to the monorepo package.json (and pinned) and that their named exports match what is imported. Also the plugin keys are string literals (e.g., 'ptm_chart', 'ptm_echarts_timeseries'); consider centralizing keys or exporting constants from the plugin package to prevent collisions and typos.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-2c05294d4e4261c20f83ea7cfe4317f08630f6c804d09e459bc1d88967f2e438R30-R43'><strong>Global locale side-effect</strong></a><br>The new code unconditionally imports the Portuguese locale and calls dayjs.locale('pt-br'), which mutates Dayjs's global locale for the entire application. This can cause unexpected localization changes for other parts of the app that expect the user's or app's configured locale. Consider making locale selection explicit/configurable or applying locale per-call instead of globally.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-1349d1df466ac2f93e7b5db79a83ab33c7e9dacab5e1649c66b332cb140e6530R18-R18'><strong>`class-properties` loose semantics</strong></a><br>The plugin `@babel/plugin-proposal-class-properties` is enabled with `{ loose: true }`. The `loose` option changes semantics (e.g., how instance properties are emitted). Confirm this is intentional — using `loose: true` can be a source of subtle bugs if consumers expect spec-compliant behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36818/files#diff-415de08cbf9e8633d6cf2c439df7aa08bf9ebce684833ee837d6ce7d7ab7d53bR38-R46'><strong>Mutating bootstrap payload & normalization</strong></a><br>`override_bootstrap_locale` mutates the incoming `data` dict in-place and only handles the exact value `"pt"`. It doesn't handle different case or formats (e.g. `"pt-br"`, `"pt_BR"`) and could unintentionally change external state. Consider normalizing and returning a new dict (non-destructive), and cover common variants.<br> </td></tr> </table> -- 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]
