codeant-ai-for-open-source[bot] commented on PR #36760: URL: https://github.com/apache/superset/pull/36760#issuecomment-3673955671
## 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/36760/files#diff-151531ca15dedfcbd010fb71d7f257b17fb5ee4cb5d8d4f308a4d9fbb49099eaR68-R71'><strong>Unsafe Default State</strong></a><br>The reducer default state is set to an empty object cast to `SqlLabState` (`{} as SqlLabState`). Casting an empty object silences missing required properties and may mask runtime errors. Use an explicit, properly shaped initial state (imported initial state) or make the reducer accept a Partial state to ensure required fields are present.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-ee0f489501fa0db67eda84a06816bb47883133eb19ec4fcc03aa958373e6527aR104-R141'><strong>Possible runtime error</strong></a><br>The code grabs a control component from `controlMap` using `this.props.controlName` without guarding against `undefined`. If `controlMap[this.props.controlName]` is missing the render will throw. Consider validating and providing a clear fallback UI or throwing a helpful error.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-406a232a48e61a087f699432b8b51b14fd99cf47fd48404d621909cdfa0deeb8R276-R279'><strong>Unsafe savedMetrics access</strong></a><br>getMetricExpression calls .expression on the result of .find(...) without checking whether a matching saved metric exists. If no saved metric matches, this will throw a runtime TypeError. Consider returning a safe default or handling the missing case explicitly.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-2b4cf963df773e4ddbfd705341760c0059a9000287b7eafd9680cbf192df66b4R130-R159'><strong>State mutation</strong></a><br>The slicer function mutates the incoming Redux state: it uses `delete` on `statePath.common` and assigns to `state.localStorageUsageInKilobytes`. Mutating the provided `state` in a Redux pipeline can cause subtle bugs and break assumptions about immutability. Consider returning derived values instead of mutating the input.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-1ae18f3051bec726f69a866ad78fe09c5f0938f08bda8d27f1f279592fbe2451R49-R74'><strong>Query id required</strong></a><br>The new `Query` interface declares `id` as required (`string`), but multiple code paths (and the original JS) allow creating queries without an `id` and rely on `startQuery` to generate one. This mismatch can produce incorrect type assumptions across callers or TypeScript errors; make `id` optional or ensure callers always set it.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-547b8e73c38ae587e3320b851ebb76bc1e318c3211e10fe649c029e905326d7eR185-R205'><strong>Unsafe array access</strong></a><br>The code calls array methods without guarding that the arrays exist. Examples: `const { columns } = datasource; columns.find(...)` and `datasource.owners?.map(...).includes(...)` can throw if `columns` or the result of the optional chain is undefined. These places should defensively handle missing `columns`/`owners` to avoid runtime exceptions.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-547b8e73c38ae587e3320b851ebb76bc1e318c3211e10fe649c029e905326d7eR301-R305'><strong>Optional chaining misuse</strong></a><br>The expression `datasource.owners?.map(...).includes(...)` uses optional chaining only for `map`, but then immediately calls `.includes` on possibly `undefined`. Use a fallback (e.g. `|| []`) or restructure to avoid calling methods on undefined values.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-0c83f58bbba7ec94b115aa8fb08a3f8041da58ee082557414128554f21f3e884R133-R145'><strong>Potential undefined entries in selectedLayers</strong></a><br>When mapping `layerFilterScope` to find matching options from `result.data` the code does not filter out not-found results. That can leave `selectedLayers` containing `undefined` entries which may be passed to the `Select` `value` prop and later cause `onSave` to include `undefined` in the saved `layerFilterScope`. Consider filtering out falsy/undefined items before calling setState or ensuring `find` always returns a valid option.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-547b8e73c38ae587e3320b851ebb76bc1e318c3211e10fe649c029e905326d7eR167-R173'><strong>Event handling bug</strong></a><br>The helper that handles Link onClick (preventRouterLinkWhileMetaClicked) appears to invert semantics: it calls preventDefault() when metaKey is true and stopPropagation() otherwise. This likely prevents the browser default (opening in a new tab) while allowing client-side router handling, which is the opposite of the intended behavior. Verify desired behavior and fix the event handling to stop router navigation while preserving default browser behavior for modifier-clicks.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-4826ab6b7f8ba33180d88a0214f2e7c4f668bbbe7c45dbdfaa2a7f91572834bbR144-R158'><strong>Unsafe optional access</strong></a><br>The implementation of `getDefaultTab` accesses `savedMetric.metric_name` without guarding for `savedMetric` being undefined. If `props.savedMetric` is undefined this will throw at runtime. The same logic is invoked when initializing `defaultActiveTabKey`, increasing the chance of a crash during component construction.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-65ee82e08144650caf5297d379cecf103fbf580b139888a9d5908a0f8c4a31eaR148-R153'><strong>Incorrect object check</strong></a><br>The code uses `Object.is(c)` to test whether a choice `c` is an object. `Object.is` is a function that requires two arguments and is not a truthy test for "is object". This will cause plain-object choices to fall through to the primitive branch, producing incorrect `{ value, label }` mapping (labels/values lost). Inspect how object choices are detected and mapped.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-b178fc7e971756ac4b6ad63f6757b9c07a10157ed2b7c48c5a58b12d804f5210R52-R56'><strong>onInit return mismatch</strong></a><br>The code returns the result of controlPanelConfig.onInit(controlsState). However, the upstream type for onInit is declared as returning void in the control panel types. If onInit returns nothing (undefined) but only mutates the passed state, this function will return undefined instead of the expected controlsState. Confirm whether onInit should return the new controls state or mutate it in-place, and make the code robust to both behaviors.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-e04ae5df3d146f2dc1db8afc14bc31e498eb8b970c077620b75f344d410eb4e7R93-R100'><strong>Falsy coordinate check</strong></a><br>The label rendering uses truthy checks for longitude and latitude. If either coordinate is 0 the check will fail and the label will show "N/A" even though valid coordinates exist. Use nullish checks or typeof checks instead.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-323381f60dadd727d9ab4a2c7f540aaa39002174c97edd2797051570bfa8f0ebR248-R265'><strong>Missing effect dependencies</strong></a><br>The effect that reconciles metrics when columns or saved metrics change references `propsValue`, `prevColumns`, and `prevSavedMetrics` but they are not listed in the dependency array. This can cause stale reads or missed updates when those values change and lead to inconsistent state or UI not updating after dataset/prop changes.<br> - [ ] <a href='https://github.com/apache/superset/pull/36760/files#diff-323381f60dadd727d9ab4a2c7f540aaa39002174c97edd2797051570bfa8f0ebR162-R180'><strong>Ambiguous conditional precedence</strong></a><br>The comparison logic in `onMetricEdit` mixes `||` and the ternary operator without explicit parentheses, which makes the intent unclear and may lead to incorrect matches between saved and adhoc metrics. This is error-prone and should be parenthesized or refactored into clearer checks.<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]
