codeant-ai-for-open-source[bot] commented on PR #36923: URL: https://github.com/apache/superset/pull/36923#issuecomment-3714268008
## 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/36923/files#diff-b8fd4567cad703641ee47670bb32dbe0e32621d0e8ac51fecbc9ba1cd161b4a3R23-R29'><strong>Behavioral change</strong></a><br>The removed guard that required `comparison_type === ComparisonType.Values` means any chart with a `time_compare` entry may now mark series as derived. Verify this doesn't enable dashed styling for other comparison types unintentionally. Confirm desired behavior across all supported `comparison_type` values.<br> - [ ] <a href='https://github.com/apache/superset/pull/36923/files#diff-42b0621f739b90a1a96c99c08d34ca095dca4259166d71586692c96c74407e6aR34-R39'><strong>getTimeOffset guard</strong></a><br>The exported helper `getTimeOffset` assumes `series.name` is a string (it calls `.includes`), but it has no internal type guard. While `hasTimeOffset` now checks the type before calling it, other call sites could call `getTimeOffset` directly and trigger a runtime error if `series.name` is undefined or non-string. Consider making `getTimeOffset` defensive about `series.name` or narrow its parameter type.<br> - [ ] <a href='https://github.com/apache/superset/pull/36923/files#diff-e32524f85caf42e0e6cb07a5a75fb37b523fda23f6a15e26608df3b76c12a4fcR36-R44'><strong>Behavior change</strong></a><br>Removing the restriction that only ComparisonType.Values marks a derived series means series whose names match time offsets will now be treated as derived regardless of the configured comparison_type. This can change visual styling (dashed vs solid lines) in other comparison modes; verify this is intended across chart types and UI surface.<br> - [ ] <a href='https://github.com/apache/superset/pull/36923/files#diff-42b0621f739b90a1a96c99c08d34ca095dca4259166d71586692c96c74407e6aR34-R40'><strong>Matching robustness</strong></a><br>The new exact-match check (`timeCompare.includes(series.name)`) in `hasTimeOffset` fixes a regression but can still miss matches due to leading/trailing whitespace or minor formatting differences. Also, logic is split between `getTimeOffset` (pattern matches) and `hasTimeOffset` (exact match), which may lead to maintenance churn. Consider centralizing and normalizing comparisons (trim/case) and consolidating matching logic.<br> - [ ] <a href='https://github.com/apache/superset/pull/36923/files#diff-e32524f85caf42e0e6cb07a5a75fb37b523fda23f6a15e26608df3b76c12a4fcR8-R12'><strong>Viz type clarity</strong></a><br>The shared `formData` uses `VizType.Table`. The regression being fixed relates to charts with no dimensions (e.g. line charts); confirm tests reflect the intended chart type or add explicit tests for relevant viz types so behavior for dimension-less charts is validated.<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]
