Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PORTALS-3364 - Fix QueryWrapper stale state bug #1466

Merged
merged 1 commit into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
Original file line number Diff line number Diff line change
@@ -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'
Expand All @@ -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, {
Expand All @@ -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'
Expand Down Expand Up @@ -308,6 +310,7 @@ function QueryWrapperPlotNav(props: QueryWrapperPlotNavProps) {
const remount = () => {
setComponentKey(componentKey + 1)
}

const { data: entity } = useGetEntity(entityId, versionNumber)
const initQueryRequest: QueryBundleRequest = {
entityId,
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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}
Expand Down
Loading