fix: Guard against unnecessary timer rerenders on /artwork/id for closed auction artworks #15053
Conversation
damassi
force-pushed
the
review-app-inp-artwork
branch
from
04b4650 to
8442c61
Compare
#1440 Bundle Size — 8.98MiB (+0.02%).fa11f36(current) vs dfdda0e main#1433(baseline) Warning Bundle contains 14 duplicate packages – View duplicate packages Bundle metrics
|
| Current #1440 |
Baseline #1433 |
|
|---|---|---|
 Initial JS |
3.66MiB(~+0.01%) |
3.66MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
77.36% |
43.2% |
| Chunks |
103 |
103 |
| Assets |
106 |
106 |
| Modules |
5842 |
5842 |
| Duplicate Modules |
529 |
529 |
| Duplicate Code |
4.02% |
4.02% |
| Packages |
266 |
266 |
| Duplicate Packages |
13 |
13 |
Bundle size by type 1 change 1 regression
| Current #1440 |
Baseline #1433 |
|
|---|---|---|
| JS |
8.84MiB (+0.02%) |
8.84MiB |
| Other |
143.36KiB |
143.36KiB |
Bundle analysis report Branch review-app-inp-artwork Project dashboard
Generated by RelativeCI Documentation Report issue
damassi
force-pushed
the
review-app-inp-artwork
branch
from
8442c61 to
67b6c5a
Compare
damassi
force-pushed
the
review-app-inp-artwork
branch
from
67b6c5a to
fa11f36
Compare
|
|
||
| const websocketEnabled = !!sale?.extendedBiddingIntervalMinutes | ||
| const websocketEnabled = | ||
| !!sale?.extendedBiddingIntervalMinutes && isLotClosed === false |
There was a problem hiding this comment.
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.
This should probably be ||
| const partnerOfferTimer = useTimer(partnerOffer?.endAt || THE_PAST) | ||
| const partnerOfferTimer = useTimer({ | ||
| endDate: partnerOffer?.endAt || THE_PAST, | ||
| enabled: !isLotClosed, |
There was a problem hiding this comment.
Shouldn't this be based on the offer rather than lot status?
There was a problem hiding this comment.
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.
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.
| } | ||
|
|
||
| export const useCurrentTime = ({ | ||
| interval = 1000, | ||
| interval = 5000, |
There was a problem hiding this comment.
This is used to display 
| @@ -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" | |||
There was a problem hiding this comment.
Finding it surprising that there's no field to determine if a lot is closed in Metaphysics
| const { hasEnded } = useTimer({ | ||
| endDate: biddingEndAt as string, | ||
| startAt: artwork?.sale?.startAt as string, | ||
| enabled: !initialIsLotClosedCheck, |
There was a problem hiding this comment.
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.
|
Disabling the timer means it won't ever initialize if the page loads before startAt. The performance impact of once-per-second renders is negligible in real-world scenarios. If we do want to optimize this, we should focus on tightly scoping the renders by encapsulating the timers and components to avoid parent context interference. |
|
Gonna close this because in the end it had no impact on INP. Thanks for the detailed comments everyone; we can return to this if / when we want to optimize. |
The type of this PR is: Fix
Description
This fixes an issue where we were unnecessarily rerendering the artwork page even if an auction artwork has closed by:
useTimeranduseCurrentTimeby adding an additionalenabledpropenabledprop to the AuctionWebsocket context and checking if we should enable itAdditionally:
yarn scanby downgrading the lib to a working version (something broke in the latest, which made it so that we couldn't see rerenders. Fixed it0.0.39cc @artsy/diamond-devs