codeant-ai-for-open-source[bot] commented on PR #36754: URL: https://github.com/apache/superset/pull/36754#issuecomment-3673234369
## 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/36754/files#diff-fde753102d842566aa987087548d81aee26d633fe2e267f66dbbe7b1dcd0a2c6R170-R176'><strong>Potential runtime error</strong></a><br>Creating a Set from the optional chain expression `schemaOptions?.map(...)` can pass `undefined` into the Set constructor and throw. Ensure `schemaOptions` is defaulted to an iterable (e.g. empty array) before mapping/creating a Set.<br> - [ ] <a href='https://github.com/apache/superset/pull/36754/files#diff-8494abea0f2da9544816dec5034230e2f152a174c63465bbee06d9306c9308c3R49-R65'><strong>Transform response robustness</strong></a><br>The new transformResponse assumes `json.result` is present and is an array of strings. If the backend returns undefined, null, or a different shape, `json.result.sort()` will throw. Also `defaultCatalog` is taken from `json.default` without validating that the default actually exists in the transformed `catalogs` list.<br> - [ ] <a href='https://github.com/apache/superset/pull/36754/files#diff-9d06475f6f1b83478541c662591c73c3e9c4f2e8655a7b40c0e1a8c980ec2867R61-R68'><strong>Unsafe transformResponse usage</strong></a><br>The transform maps `json.result.sort().map(...)` without checking that `json.result` exists and is an array. This can throw if the API response shape changes or is empty. Also the default `sort()` is case-sensitive and locale dependent; consider normalizing or providing a comparator.<br> - [ ] <a href='https://github.com/apache/superset/pull/36754/files#diff-8494abea0f2da9544816dec5034230e2f152a174c63465bbee06d9306c9308c3R100-R112'><strong>Error handling race / wrong error source</strong></a><br>The fetch path calls `trigger(...).then(({ isSuccess, isError, data }) => { ... })` and when `isError` it calls `onError?.(result.error as ClientErrorObject)`. Using `result.error` (the hook state) can be stale or unrelated to this trigger call. Prefer consuming the `error` returned by the trigger promise so the correct error is reported.<br> - [ ] <a href='https://github.com/apache/superset/pull/36754/files#diff-9d06475f6f1b83478541c662591c73c3e9c4f2e8655a7b40c0e1a8c980ec2867R111-R121'><strong>Incorrect error passed to onError</strong></a><br>Inside the trigger promise resolution, when the triggered request fails the code calls the outer hook's `result.error` instead of using the error returned by the trigger call. That can surface an unrelated error (or undefined) to the provided `onError` callback. Prefer using the trigger response's error payload.<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]
