From 3ef8a104c9fb3bb54a3b59a651a67e1ccb188561 Mon Sep 17 00:00:00 2001
From: Nick Grosenbacher <nickgrosenbacher@gmail.com>
Date: Tue, 17 Dec 2024 08:57:22 -0500
Subject: [PATCH] PORTALS-3364 - Fix QueryWrapper stale state bug

- QueryWrapper component should always be reset via key when the initial query changes.

See https://legacy.reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key
---
 .../CardContainerLogic/CardContainerLogic.tsx | 15 ++++++-
 .../QueryPerFacetPlotsCard.tsx                | 15 ++++++-
 .../SingleQueryFacetPlotsCards.tsx            | 11 ++++-
 .../QueryWrapperPlotNav.tsx                   | 21 +++++++---
 .../StandaloneQueryWrapper.tsx                | 42 ++++++++++++-------
 5 files changed, 80 insertions(+), 24 deletions(-)

diff --git a/packages/synapse-react-client/src/components/CardContainerLogic/CardContainerLogic.tsx b/packages/synapse-react-client/src/components/CardContainerLogic/CardContainerLogic.tsx
index 3598953fb9..981fd62459 100644
--- a/packages/synapse-react-client/src/components/CardContainerLogic/CardContainerLogic.tsx
+++ b/packages/synapse-react-client/src/components/CardContainerLogic/CardContainerLogic.tsx
@@ -211,8 +211,21 @@ export function CardContainerLogic(props: CardContainerLogicProps) {
       SynapseConstants.BUNDLE_MASK_LAST_UPDATED_ON,
   }
 
+  /**
+   * Fully re-render the uncontrolled QueryWrapper component when the initial query changes. This eliminates a class of
+   * bugs where our 'derived' state (the current query), which should be reset, is out of sync with props.
+   *
+   * See https://legacy.reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key
+   */
+  const queryWrapperKey = JSON.stringify(initQueryRequest)
+
   return (
-    <QueryWrapper {...props} initQueryRequest={initQueryRequest} isInfinite>
+    <QueryWrapper
+      {...props}
+      initQueryRequest={initQueryRequest}
+      isInfinite
+      key={queryWrapperKey}
+    >
       <QueryVisualizationWrapper
         rgbIndex={props.rgbIndex}
         unitDescription={props.unitDescription}
diff --git a/packages/synapse-react-client/src/components/FeaturedDataTabs/QueryPerFacetPlotsCard.tsx b/packages/synapse-react-client/src/components/FeaturedDataTabs/QueryPerFacetPlotsCard.tsx
index 52e69ed0bb..fa0c94eeda 100644
--- a/packages/synapse-react-client/src/components/FeaturedDataTabs/QueryPerFacetPlotsCard.tsx
+++ b/packages/synapse-react-client/src/components/FeaturedDataTabs/QueryPerFacetPlotsCard.tsx
@@ -65,8 +65,21 @@ function QueryPerFacetPlotsCard(props: QueryPerFacetPlotsCardProps) {
     selectFacetColumnName,
     selectFacetColumnValue,
   )
+
+  /**
+   * Fully re-render the uncontrolled QueryWrapper component when the initial query changes. This eliminates a class of
+   * bugs where our 'derived' state (the current query), which should be reset, is out of sync with props.
+   *
+   * See https://legacy.reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key
+   */
+  const queryWrapperKey = JSON.stringify(initQueryRequest)
+
   return (
-    <QueryWrapper {...rest} initQueryRequest={initQueryRequest}>
+    <QueryWrapper
+      {...rest}
+      initQueryRequest={initQueryRequest}
+      key={queryWrapperKey}
+    >
       <QueryVisualizationWrapper rgbIndex={rgbIndex} {...rest}>
         <QueryWrapperErrorBoundary>
           <FacetPlotsCard
diff --git a/packages/synapse-react-client/src/components/FeaturedDataTabs/SingleQueryFacetPlotsCards.tsx b/packages/synapse-react-client/src/components/FeaturedDataTabs/SingleQueryFacetPlotsCards.tsx
index 96a54101ec..9244131214 100644
--- a/packages/synapse-react-client/src/components/FeaturedDataTabs/SingleQueryFacetPlotsCards.tsx
+++ b/packages/synapse-react-client/src/components/FeaturedDataTabs/SingleQueryFacetPlotsCards.tsx
@@ -38,8 +38,17 @@ function getQueryRequest(sql: string): QueryBundleRequest {
 function SingleQueryFacetPlotsCards(props: SingleQueryFacetPlotsCardsProps) {
   const { sql, facetsToPlot, rgbIndex, columnAliases, unitDescription } = props
   const initQueryRequest: QueryBundleRequest = getQueryRequest(sql!)
+
+  /**
+   * Fully re-render the uncontrolled QueryWrapper component when the initial query changes. This eliminates a class of
+   * bugs where our 'derived' state (the current query), which should be reset, is out of sync with props.
+   *
+   * See https://legacy.reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key
+   */
+  const queryWrapperKey = JSON.stringify(initQueryRequest)
+
   return (
-    <QueryWrapper initQueryRequest={initQueryRequest}>
+    <QueryWrapper initQueryRequest={initQueryRequest} key={queryWrapperKey}>
       <QueryVisualizationWrapper
         rgbIndex={rgbIndex}
         columnAliases={columnAliases}
diff --git a/packages/synapse-react-client/src/components/QueryWrapperPlotNav/QueryWrapperPlotNav.tsx b/packages/synapse-react-client/src/components/QueryWrapperPlotNav/QueryWrapperPlotNav.tsx
index 422a03d655..87b9d15f91 100644
--- a/packages/synapse-react-client/src/components/QueryWrapperPlotNav/QueryWrapperPlotNav.tsx
+++ b/packages/synapse-react-client/src/components/QueryWrapperPlotNav/QueryWrapperPlotNav.tsx
@@ -1,5 +1,6 @@
 import { Box } from '@mui/material'
 import { Query, QueryBundleRequest } from '@sage-bionetworks/synapse-types'
+import { useQuery } from '@tanstack/react-query'
 import { useAtomValue } from 'jotai'
 import { useState } from 'react'
 import { useGetEntity } from '../../synapse-queries'
@@ -26,8 +27,10 @@ import {
 import { QueryWrapper, QueryWrapperProps } from '../QueryWrapper'
 import { isRowSelectionVisibleAtom } from '../QueryWrapper/TableRowSelectionState'
 import { QueryWrapperErrorBoundary } from '../QueryWrapperErrorBoundary'
-import { SynapseTableConfiguration } from '../SynapseTable'
-import { NoContentPlaceholderType } from '../SynapseTable'
+import {
+  NoContentPlaceholderType,
+  SynapseTableConfiguration,
+} from '../SynapseTable'
 import SearchV2, { SearchV2Props } from '../SynapseTable/SearchV2'
 import SqlEditor from '../SynapseTable/SqlEditor'
 import TopLevelControls, {
@@ -40,9 +43,8 @@ import PlotsContainer, {
 import FacetFilterControls, {
   FacetFilterControlsProps,
 } from '../widgets/query-filter/FacetFilterControls'
-import { RowSetView } from './RowSetView'
 import { QueryWrapperSynapsePlotProps } from './QueryWrapperSynapsePlot'
-import { useQuery } from '@tanstack/react-query'
+import { RowSetView } from './RowSetView'
 
 export const QUERY_FILTERS_EXPANDED_CSS: string = 'isShowingFacetFilters'
 export const QUERY_FILTERS_COLLAPSED_CSS: string = 'isHidingFacetFilters'
@@ -308,6 +310,7 @@ function QueryWrapperPlotNav(props: QueryWrapperPlotNavProps) {
   const remount = () => {
     setComponentKey(componentKey + 1)
   }
+
   const { data: entity } = useGetEntity(entityId, versionNumber)
   const initQueryRequest: QueryBundleRequest = {
     entityId,
@@ -326,11 +329,19 @@ function QueryWrapperPlotNav(props: QueryWrapperPlotNavProps) {
   const isFullTextSearchEnabled =
     (entity && isTable(entity) && entity.isSearchEnabled) ?? false
 
+  /**
+   * Fully re-render the uncontrolled QueryWrapper component when the initial query changes. This eliminates a class of
+   * bugs where our 'derived' state (the current query), which should be reset, is out of sync with props.
+   *
+   * See https://legacy.reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key
+   */
+  const queryWrapperKey = `${JSON.stringify(initQueryRequest)}-${componentKey}`
+
   return (
     <QueryWrapper
       {...props}
       initQueryRequest={initQueryRequest}
-      key={componentKey}
+      key={queryWrapperKey}
       isInfinite={isInfinite}
     >
       <QueryVisualizationWrapper
diff --git a/packages/synapse-react-client/src/components/StandaloneQueryWrapper/StandaloneQueryWrapper.tsx b/packages/synapse-react-client/src/components/StandaloneQueryWrapper/StandaloneQueryWrapper.tsx
index 8233e3363a..bcdee1dbf9 100644
--- a/packages/synapse-react-client/src/components/StandaloneQueryWrapper/StandaloneQueryWrapper.tsx
+++ b/packages/synapse-react-client/src/components/StandaloneQueryWrapper/StandaloneQueryWrapper.tsx
@@ -1,34 +1,34 @@
+import { QueryBundleRequest } from '@sage-bionetworks/synapse-types'
 import { useState } from 'react'
+import { useGetEntity } from '../../synapse-queries/entity/useEntity'
+import { SynapseConstants } from '../../utils'
+import { isTable } from '../../utils/functions/EntityTypeUtils'
 import {
   getAdditionalFilters,
   parseEntityIdFromSqlStatement,
 } from '../../utils/functions/SqlFunctions'
-import { SynapseTableConfiguration } from '../SynapseTable/SynapseTable'
-import { QueryBundleRequest } from '@sage-bionetworks/synapse-types'
-import { SynapseConstants } from '../../utils'
-import { QueryWrapper, QueryWrapperProps } from '../QueryWrapper/QueryWrapper'
-import { QueryContextConsumer } from '../QueryContext/QueryContext'
-import TopLevelControls, {
-  TopLevelControlsProps,
-} from '../SynapseTable/TopLevelControls/TopLevelControls'
+import { DEFAULT_PAGE_SIZE } from '../../utils/SynapseConstants'
 import FullTextSearch from '../FullTextSearch/FullTextSearch'
-import SearchV2, { SearchV2Props } from '../SynapseTable/SearchV2'
-import { useGetEntity } from '../../synapse-queries/entity/useEntity'
-import TotalQueryResults from '../TotalQueryResults'
-import SqlEditor from '../SynapseTable/SqlEditor'
+import { QueryContextConsumer } from '../QueryContext/QueryContext'
 import {
   QueryVisualizationContextConsumer,
   QueryVisualizationWrapper,
   QueryVisualizationWrapperProps,
 } from '../QueryVisualizationWrapper'
-import { isTable } from '../../utils/functions/EntityTypeUtils'
-import { NoContentPlaceholderType } from '../SynapseTable/NoContentPlaceholderType'
-import { DEFAULT_PAGE_SIZE } from '../../utils/SynapseConstants'
+import { QueryWrapper, QueryWrapperProps } from '../QueryWrapper/QueryWrapper'
 import {
   Operator,
   SearchParams,
 } from '../QueryWrapperPlotNav/QueryWrapperPlotNav'
 import { RowSetView } from '../QueryWrapperPlotNav/RowSetView'
+import { NoContentPlaceholderType } from '../SynapseTable/NoContentPlaceholderType'
+import SearchV2, { SearchV2Props } from '../SynapseTable/SearchV2'
+import SqlEditor from '../SynapseTable/SqlEditor'
+import { SynapseTableConfiguration } from '../SynapseTable/SynapseTable'
+import TopLevelControls, {
+  TopLevelControlsProps,
+} from '../SynapseTable/TopLevelControls/TopLevelControls'
+import TotalQueryResults from '../TotalQueryResults'
 
 type StandaloneQueryWrapperOwnProps = {
   sql: string
@@ -117,11 +117,21 @@ function StandaloneQueryWrapper(props: StandaloneQueryWrapperProps) {
     )
 
   const { data: entity } = useGetEntity(entityId)
+
+  /**
+   * Fully re-render the uncontrolled QueryWrapper component when the initial query changes. This eliminates a class of
+   * bugs where our 'derived' state (the current query), which should be reset, is out of sync with props.
+   *
+   * See https://legacy.reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key
+   */
+  const queryWrapperKey =
+    JSON.stringify(derivedQueryRequestFromSearchParams) + componentKey
+
   return (
     <QueryWrapper
       {...rest}
       initQueryRequest={derivedQueryRequestFromSearchParams}
-      key={componentKey}
+      key={queryWrapperKey}
     >
       <QueryVisualizationWrapper
         rgbIndex={rgbIndex}