codeant-ai-for-open-source[bot] commented on PR #36708: URL: https://github.com/apache/superset/pull/36708#issuecomment-3716998943
## 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/36708/files#diff-a612babd68b1abfd9dd4ebeff1bec35c7512602e35846c4058aacbac65c6c7beR595-R606'><strong>Response shape assumption</strong></a><br>The code assumes the shape of the SupersetClient.get response and that `response.json.result.params` exists and is a JSON string. If the GET call returns a different shape (e.g., `json`/`response` destructured, or `result` is an array / missing `params`), JSON.parse will throw or a runtime error will occur. Validate and guard the response before parsing.<br> - [ ] <a href='https://github.com/apache/superset/pull/36708/files#diff-70fd91701dacddf40236868a86e1fc46ce2d9ef77611d54d55b710e66c963ce9R67-R69'><strong>Mutation / shared reference risk</strong></a><br>Assigning `form_data: state.latestQueryFormData` may copy a reference to an object that is shared elsewhere. If consumers mutate `form_data` in-place it can cause unexpected cross-component state mutations. Consider making a defensive copy when writing `form_data` to the reducer output to avoid shared-reference bugs.<br> - [ ] <a href='https://github.com/apache/superset/pull/36708/files#diff-70fd91701dacddf40236868a86e1fc46ce2d9ef77611d54d55b710e66c963ce9R67-R69'><strong>Typing / property consistency</strong></a><br>The reducer adds a `form_data` property while the ChartState object, and other codepaths, use `latestQueryFormData`, `sliceFormData`, etc. This can create a typing mismatch or duplication of state fields. Verify that consumers expect `form_data` and that types remain consistent across the codebase.<br> - [ ] <a href='https://github.com/apache/superset/pull/36708/files#diff-a612babd68b1abfd9dd4ebeff1bec35c7512602e35846c4058aacbac65c6c7beR608-R613'><strong>Logging and user feedback</strong></a><br>On fetch failure the code uses `console.warn` to log and falls back silently to cached data. This may be missed in production logs and provides no user-facing feedback. Prefer using the app's logger or dispatching a toast so operators/users can be informed and logs are centralized.<br> - [ ] <a href='https://github.com/apache/superset/pull/36708/files#diff-a612babd68b1abfd9dd4ebeff1bec35c7512602e35846c4058aacbac65c6c7beR595-R614'><strong>Latency / UX concern</strong></a><br>The refreshChart action now performs an additional network request to fetch the chart definition on every forced refresh. This can add latency to the refresh path and may be redundant if the cached `latestQueryFormData` is already up-to-date. Consider conditional fetching (e.g., based on a last-updated timestamp or etag) or making this fetch optional.<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]
