-
Notifications
You must be signed in to change notification settings - Fork 153
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
fix: Guard against unnecessary timer rerenders on /artwork/id for closed auction artworks #15053
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import { ArtworkErrorApp } from "Apps/Artwork/Components/ArtworkErrorApp/Artwork | |
import { ArtworkPageBanner } from "Apps/Artwork/Components/ArtworkPageBanner" | ||
import { PrivateArtworkDetails } from "Apps/Artwork/Components/PrivateArtwork/PrivateArtworkDetails" | ||
import { SelectedEditionSetProvider } from "Apps/Artwork/Components/SelectedEditionSetContext" | ||
import { lotIsClosed } from "Apps/Artwork/Utils/lotIsClosed" | ||
import { AlertProvider } from "Components/Alert/AlertProvider" | ||
import { useAuthDialog } from "Components/AuthDialog" | ||
import { RecentlyViewed } from "Components/RecentlyViewed" | ||
|
@@ -334,7 +335,14 @@ export const ArtworkApp: React.FC<React.PropsWithChildren<Props>> = props => { | |
|
||
const WrappedArtworkApp: React.FC<React.PropsWithChildren<Props>> = props => { | ||
const { | ||
artwork: { artists, attributionClass, internalID, mediumType, sale }, | ||
artwork: { | ||
artists, | ||
attributionClass, | ||
internalID, | ||
mediumType, | ||
sale, | ||
saleArtwork, | ||
}, | ||
} = props | ||
|
||
const { | ||
|
@@ -346,8 +354,10 @@ const WrappedArtworkApp: React.FC<React.PropsWithChildren<Props>> = props => { | |
// Check to see if referrer comes from link interception. | ||
// @see interceptLinks.ts | ||
const referrer = state && state.previousHref | ||
const isLotClosed = lotIsClosed(sale, saleArtwork) | ||
|
||
const websocketEnabled = !!sale?.extendedBiddingIntervalMinutes | ||
const websocketEnabled = | ||
!!sale?.extendedBiddingIntervalMinutes && isLotClosed === false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haven't totally grokked this, but does this imply that a sale without extended bidding will always skip polling? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be || |
||
|
||
const initialAlertCriteria = { | ||
attributionClass: compact([attributionClass?.internalID]), | ||
|
@@ -433,6 +443,10 @@ const ArtworkAppFragmentContainer = createFragmentContainer( | |
internalID | ||
slug | ||
extendedBiddingIntervalMinutes | ||
isClosed | ||
} | ||
saleArtwork { | ||
endedAt | ||
} | ||
saleMessage | ||
artists(shallow: true) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,16 +38,20 @@ const ArtworkAuctionCreateAlertHeader: FC< | |
> = ({ artwork }) => { | ||
const biddingEndAt = | ||
artwork?.saleArtwork?.extendedBiddingEndAt ?? artwork?.saleArtwork?.endAt | ||
const { hasEnded } = useTimer( | ||
biddingEndAt as string, | ||
artwork?.sale?.startAt as string, | ||
) | ||
|
||
const initialIsLotClosedCheck = lotIsClosed(artwork.sale, artwork.saleArtwork) | ||
|
||
const { hasEnded } = useTimer({ | ||
endDate: biddingEndAt as string, | ||
startAt: artwork?.sale?.startAt as string, | ||
enabled: !initialIsLotClosedCheck, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing about this entire approach is that we're not polling on the status right? So if you load the page before it starts the status will be closed and then if startAt comes to pass it will never enable. |
||
}) | ||
|
||
const isLotClosed = hasEnded || lotIsClosed(artwork.sale, artwork.saleArtwork) | ||
const displayAuctionCreateAlertHeader = | ||
artwork.isEligibleToCreateAlert && artwork.isInAuction && isLotClosed | ||
|
||
const artistName = artwork.artistNames ? ", " + artwork.artistNames : "" | ||
const artistName = artwork.artistNames ? `, ${artwork.artistNames}` : "" | ||
const artistSlug = artwork.artists?.[0]?.slug | ||
let aggregations: Aggregations = [] | ||
let additionalGeneIDs: string[] = [] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ import { | |
} from "./ArtworkSidebarEditionSets" | ||
|
||
import { useSelectedEditionSetContext } from "Apps/Artwork/Components/SelectedEditionSetContext" | ||
import { lotIsClosed } from "Apps/Artwork/Utils/lotIsClosed" | ||
import { usePartnerOfferCheckoutMutation } from "Apps/PartnerOffer/Routes/Mutations/UsePartnerOfferCheckoutMutation" | ||
import { CreateAlertButton } from "Components/Alert/Components/CreateAlertButton" | ||
import { useAuthDialog } from "Components/AuthDialog" | ||
|
@@ -71,8 +72,13 @@ export const ArtworkSidebarCommercialButtons: React.FC< | |
extractNodes(me.partnerOffersConnection)[0]) || | ||
null | ||
|
||
const isLotClosed = lotIsClosed(artwork.sale, artwork.saleArtwork) | ||
|
||
// Fall back to a definitely past value because the timer hook doesn't like nulls | ||
const partnerOfferTimer = useTimer(partnerOffer?.endAt || THE_PAST) | ||
const partnerOfferTimer = useTimer({ | ||
endDate: partnerOffer?.endAt || THE_PAST, | ||
enabled: !isLotClosed, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be based on the offer rather than lot status? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just set up so that if the lot is closed, we don't poll. I'm not too sure beyond that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me that we can just have use timer disable itself once it reaches the end date. And then we wouldn't need to explicitly enable or disable in these cases. |
||
}) | ||
const partnerIcon = artwork.partner?.profile?.icon?.url | ||
|
||
const activePartnerOffer = | ||
|
@@ -329,6 +335,7 @@ export const ArtworkSidebarCommercialButtons: React.FC< | |
const { setSelectedEditionSet: setSelectedEditionSetInContext } = | ||
useSelectedEditionSetContext() | ||
|
||
// biome-ignore lint/correctness/useExhaustiveDependencies: <explanation> | ||
useEffect(() => { | ||
setSelectedEditionSet(firstAvailableEcommerceEditionSet()) | ||
setSelectedEditionSetInContext( | ||
|
@@ -353,7 +360,9 @@ export const ArtworkSidebarCommercialButtons: React.FC< | |
} | ||
if (artwork.isOfferable && !(activePartnerOffer && artwork.isInquireable)) { | ||
renderButtons.makeOffer = | ||
Object.keys(renderButtons).length == 0 ? "primaryBlack" : "secondaryBlack" | ||
Object.keys(renderButtons).length === 0 | ||
? "primaryBlack" | ||
: "secondaryBlack" | ||
} | ||
if (artwork.isInquireable && Object.keys(renderButtons).length < 2) { | ||
renderButtons.contactGallery = | ||
|
@@ -699,6 +708,12 @@ const ARTWORK_FRAGMENT = graphql` | |
collectorSignals { | ||
primaryLabel(ignore: [PARTNER_OFFER]) | ||
} | ||
sale { | ||
isClosed | ||
} | ||
saleArtwork { | ||
endedAt | ||
} | ||
} | ||
` | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finding it surprising that there's no field to determine if a lot is closed in Metaphysics