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

site: do not get icon repeatedly #2163

Merged
merged 1 commit into from Feb 26, 2023
Merged

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Feb 23, 2023

A variety of notifications from the client backend cause the frontend to update an asset's "button" on the wallet's page. In part, this involves setting tmpl.img.src, which unfortunately initiates a GET request for the resource, whether the path has changed or not.

For instance, my testnet bch wallet was spewing wallet state notes, causing repeated fetches of the bch.png icon each time:

image

If the browser isn't sharp about caching these resources, it hits the backend repeatedly:

2023/02/23 13:54:17 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 12.844756ms
2023/02/23 13:54:20 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 12.682972ms
2023/02/23 13:54:23 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 13.716873ms
2023/02/23 13:54:26 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 12.674457ms
2023/02/23 13:54:29 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 11.653743ms
2023/02/23 13:54:32 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 8.95034ms
2023/02/23 13:54:35 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 9.302125ms
2023/02/23 13:54:38 "GET http://127.0.0.2:5758/img/coins/bch.png HTTP/1.1" from 127.0.0.1:54706 - 200 4103B in 14.660306ms

This PR updates the updateAssetButton method of WalletsPage so that it does not re-assign the img.src field if it has already been set. This method is called by at least 4 notification handlers, so it is the main offender.

@chappjc chappjc added this to the 0.6 milestone Feb 24, 2023
@@ -603,7 +603,7 @@ export default class WalletsPage extends BasePage {
const { bttn, tmpl } = this.assetButtons[assetID]
Doc.hide(tmpl.fiat, tmpl.noWallet)
bttn.classList.add('nowallet')
tmpl.img.src = Doc.logoPath(a.symbol)
tmpl.img.src ||= Doc.logoPath(a.symbol) // don't initiate GET if already set (e.g. update on some notification)
Copy link
Member Author

@chappjc chappjc Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first did this with the nullish coalescing assignment operator (??=) but to my surprise the src attribute is never null for an img tag, even if it's not there in the html (as with our template); it's just empty, so we have to go with the falsy condition.

@buck54321
Copy link
Member

We could also leverage the Expires or Cache-Control headers for images, so even if a request comes through, the response will be tiny.

@chappjc
Copy link
Member Author

chappjc commented Feb 25, 2023

We could also leverage the Expires or Cache-Control headers for images, so even if a request comes through, the response will be tiny.

Yup, I had a second commit I was going to push with this PR, but just this change seemed to do the trick (when the browser doesn't have the "Disable cache" box checked :embarrased:).

3769c4c

@chappjc chappjc merged commit 424d6d2 into decred:master Feb 26, 2023
@chappjc chappjc deleted the no-reget-coin-logo branch February 26, 2023 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants