fix: Missing ownState and isCached props in Chart.jsx#34259
Conversation
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Unnecessary Memoization of Simple Calculation ▹ view | 🧠 Not in scope |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/dashboard/components/gridComponents/Chart.jsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| const isCached = useMemo( | ||
| // eslint-disable-next-line camelcase | ||
| () => queriesResponse?.map(({ is_cached }) => is_cached) || [], | ||
| [queriesResponse], | ||
| ); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Not unnecessary, .map creates a new array every time, so every usage of isCached in dependency arrays would break memoizations
There was a problem hiding this comment.
You're right - the memoization is needed here to maintain referential equality of the mapped array. I retract my suggestion.
|
@kgabryje Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@kgabryje Ephemeral environment spinning up at http://35.92.236.9:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
SUMMARY
After changes from PR #31241,
props.ownStateandprops.isCachedwere undefined. This PR fixes the issue by usingownStateandisCachedvalues calculated within the component.Thanks @rad-pat for spotting the problem!
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION