Turbolinks-integrated query parameters and stimulus integration on address page #845

buck54321 · 2018-11-28T18:46:28Z

Addresses #814, #815.

The primary issue is that history.replaceState is not compatible with Turbolinks' hijacked browser navigation. This started as an attempt to simply replace the history in a way that Turbolinks could understand, but after seeing some deficiencies in the way the anchor text was being handled, it has expanded beyond that a little bit. I've tried to make the solution portable for inclusion in other pages as well.

What this solves

Forward and backward buttons will take you to and from a page as expected
More customization of url anchor strings allow more precise chart parameters. You can have a link to exactly the chart you want now, including a zoom level and bin size.

What's not working

Right now, the forward and back buttons take you to the page, but the view settings revert to the original settings (view, zoom, bin) from when you first navigated to the page (by link or omnibar), rather than the last state you left it in. I think this is related to Turbolinks caching, but haven't dug into it yet.
Probably other stuff. Needs some testing.

@gozart and @ZeroASIC. If you could find time to fiddle with this a little, I would appreciate it.

public/js/controllers/address_controller.js

chappjc · 2018-11-29T16:25:51Z

We'll still need a simple solution to the pushState issue that got into the address charts page on 3.1-stable. Is there something any better than simply removing the following line from charts.js?

dcrdata/public/js/controllers/charts.js

Line 201 in 4b99577

window.history.pushState({}, chartName, `#${chartName}`);

Removing it would of course mean the anchors don't go to the URL when changing the view/chart, but at least forward/back nav should work, right?

buck54321 · 2018-11-29T17:23:34Z

Yep. Removing it will be fine, though the url fragment will no longer change when you change the view or chart type.

If you want to change the fragment, use can replace pushState with

Turbolinks.controller.replaceHistoryWithLocationAndRestorationIdentifier(Turbolinks.Location.wrap(`#${chartName}`), Turbolinks.uuid())

ZeroASIC · 2018-11-29T17:24:30Z

Yeah the basic navigation in the browser should always work. I'm still waiting for my dcrdata VM to finish syncing. Once it's done I'll take a closer look at these issues.

chappjc · 2018-11-29T18:02:23Z

Thanks, @ZeroASIC. Be sure to get postgres tuned up to save tons of time. There's some tips in a conf file in db/dcrpg.

ZeroASIC · 2018-11-30T22:34:27Z

Okay, looking this over you did get the navigation working correctly, but you've duplicated functionality that already exists in Javascript URLSearchParams so you can use anchors. Anchors weren't really meant to be used this way anyhow so I think it would be best if the anchors were removed from this page completely and replaced with URL Parameters.
https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams

Also think about storing settings in the cookie and the url so they can be retrieved on page load if they're not in the URL already or the cookie can be rebuilt off of the URL.

Edit: That link says it might not work in Safari, but Apple says it has partial support: https://developer.apple.com/documentation/webkitjs/urlsearchparams

buck54321 · 2018-11-30T23:33:24Z

I agree that things would be easier if we use the query parameters. Please note that we do have two different things happening though. Right now, the query parameters are used for list pagination, which causes a Turbolinks visit. Changing chart parameters does not cause a Turbolinks visit, so maybe that's why the chart parameter was put in the anchor to begin with.

I'd very much prefer to use query parameters, though, so if nobody objects, I'll work in that direction.

ZeroASIC · 2018-11-30T23:50:47Z

I get ya. So #types stayes, but everything else goes into a query parameter and Turbolinks keeps working too. Sounds good to me.

chappjc · 2018-12-03T12:59:05Z

That plan sounds good @buck54321. Please keep 3.1 in mind though since it has to backport smoothly.

buck54321 · 2018-12-03T15:48:13Z

Switched everything to query parameters and moved the inline JS to stimulus.

@chappjc This is a rather large change that would probably be difficult to cherry pick to 3.1-stable. Can I do a separate limited PR on that branch.

public/js/controllers/address_controller.js

chappjc

I know this is still WIP, but just a few issues I noticed:

This doesn't matter too much, but when clicking "Chart". then back to "List", the &bin=all query part that is added for the Chart view stays in the URL.
When in the Chart view for an address with few transactions, clicking the "All Blocks" grouping (the only enabled grouping button) shows the List view. The Charts button is still highlighted, but the grouping buttons are gone. EDIT: this happens for any address actually.
Then after the previous point, the page freezes up and browser CPU goes high (testing with chrome). I might have clicked the List Button at this point since the Charts button was incorrectly highlighted. The associated message in the console after the browser recovers:

Active resource loading counts reached a per-frame limit while the tab was in background. Network requests will be delayed until a previous loading finishes, or the tab is brought to the foreground. See https://www.chromestatus.com/feature/5527160148197376 for more details

The sent/received/net check boxes are not working right. They ended up on the Transaction Types view at some point, and they wouldn't work there of course. This might have been fallout from the previous issue though.
I got this while clicking around. Sorry I don't know what triggered it:

    at u.computeCombinedSeriesAndLimits_ (<anonymous>:5:25502)
    at u.drawMiniPlot_ (<anonymous>:5:24051)
    at u.drawStaticLayer_ (<anonymous>:5:23450)
    at u.renderStaticLayer_ (<anonymous>:5:17362)
    at Q.cascadeEvents_ (<anonymous>:3:25195)
    at Q.predraw_ (<anonymous>:4:12117)
    at Q.setVisibility (<anonymous>:4:27111)
    at e.updateFlow (app.bundle.js:1)
    at e.processData (app.bundle.js:1)
    at e.drawGraph (app.bundle.js:1)

The chart type drop down list box does not get set to the right value when loading/reloading a URL with a specific view (e.g. ?view=unspent loads the page with the correct chart, but "Transaction Types" selected).

chappjc · 2018-12-04T16:05:32Z

When looking at this for 3.1, we should consider the simplest possible fix. If that means removing the pushState entirely and not introducing TurboQuery, then we should do that. However, if there are small tweaks that will at least make the navigation arrows not broken in 3.1, then we should make those changes.

gozart1 · 2018-12-06T10:12:12Z

public/js/controllers/address_controller.js

  }

  initialize () {
-    var _this = this
+    var controller = this


Instead of setting this onto a variable to pass it into new scope, we can just use fat arrow functions. ( since their function bodies have lexical scoping of this, see https://toddmotto.com/es6-arrow-functions-syntaxes-and-lexical-scoping/#functionality-lexical-scoping-this )

So by using fat arrow function in the BLOCK_RECIEVED handler below, this would be the controller and the alias variable not necessary.

I mostly do the controller thing for semantics (which is why _this just wasn't working for me), but arrow functions are definitely appropriate in many places.

Just so I know, now that you have the ability, were we planning on using a babel plugin to translate arrow functions?

Production builds already transpile => to function via https://babeljs.io/docs/en/babel-preset-env being set in our webpack prod configuration.
https://github.com/decred/dcrdata/blob/master/webpack.prod.js#L27

This preset essentially says to transpile everything to ES5 that can be transpiled without polyfills.

Some things can't be transpiled, like async/await, so we need to import a polyfill for those https://github.com/decred/dcrdata/blob/master/public/index.js#L1 ( regenerator-runtime is the polyfill for async await )

buck54321 · 2018-12-09T21:17:05Z

A set of optimizations for the address page. Page load times reduced from > 14 to ~ 2.25 seconds on my server.

Removes AddressMetrics collection. Instead, an initial set of chart data is collected, in the background if the user is on the list tab, and the bin and zoom buttons are selectively hidden based on the incoming data by looking at it's time range. In addition to the load time improvement, collecting the chart data is also faster than AddressMetrics.
Graph data cached. E.g. changing bin size from week to month and back to week doesn't incur an additional request. The cache is dumped on controller disconnect.
List pagination is handled with ajax. While a Turbolinks visit is fast enought for list pagination alone, the chart component sometimes requires a substantial amount of data, so preventing additional requests is critical.
Full query parameter integration. Zoom, bin, chart type, sent/net/received checkboxes, and list pagination parameters can all be passed as query parameters. Additionally, they are updated as you change the associated inputs on the page. Sharing the url from the omnibar will bring a user to the exact same view currently displayed.
Inline scripts removed. Complete stimulus integration. Very little jQuery remains.
Dygraph script is only loaded when needed. Because we use Turbolinks, and Dygraph is global, there was no need to load it with the controller on every connection.
Forward and back browser navigation fixed.

Other than the project treasury, check out these addresses of varying activity level.

Dca3BGMZPYDBPVFMG66aopfgSzNxS2JxjV1 : 131 i/o
Dca71H3XmNkGgyuNacxaJLAnbxgAYU6g3JC : 449 i/o
Dca8bQ4UX3ZKFkNTbUkmHbrfdhur1sPSNfL : 3,314 i/o
DccJwfrmQYrUNjnbECoeWHNfuwxfFy39bNp : 27,218 i/o

Resolves #835, #643. Also part of #843.

There is one weird bug (also on master) that I have yet to resolve. When loading the project treasury "all blocks" data, Dygraph hangs and the page becomes unresponsive. That alone might not be too unexpected. It is hundreds of thousands of data points. But sometimes, for some reason, the websocket connection is lost in the process.

chappjc

No major issues, just some concerns about a few explorer -> dbtypes moves. Nice job slaying inline JS!

explorer/explorerroutes.go

views/address.tmpl

db/dbtypes/types.go

db/dcrpg/pgblockchain.go

db/dcrsqlite/apisource.go

chappjc · 2018-12-10T15:50:59Z

explorer/explorerroutes.go

@@ -1586,6 +1420,8 @@ func (exp *explorerUI) Search(w http.ResponseWriter, r *http.Request) {

 	// Try aux DB if in full mode.
 	if !exp.liteMode {
+		// This is be unnecessarily duplicative and possible very
+		// slow for a very active addresss.


I'd like to personally follow up on this PR because the call to exp.blockData.GetExplorerAddress several lines up is no longer appropriate above. It was simply used to validate the address, but since adding txhelpers.AddressValidation, that's what should be done above (avoiding the expensive SearchRawTransactoinVerbose RPC).

The purpose of this "Try aux DB if in full mode" when the RPC was just used above was because transactions for orphaned blocks may not show up in the output of searchrawtransactions, but PostgreSQL may have recorded data for the transaction and associated addresses. This is the same reasoning for checking PostgreSQL for transactions with exp.explorerSource.Transaction on this line below.

However, Search does not need to do the address queries at all. That is the job of the address page. Here we only need to do txhelpers.AddressValidation (as done in GetExplorerAddress and elsewhere) and redirect to the appropriate /address URL.

Anyway, I can pick this up after your PR since it's a Search page issue.

rpcutils/rpcclient.go

gozart1 · 2018-12-11T09:00:44Z

public/js/controllers/address_controller.js

+      'flow', 'zoom', 'interval', 'numUnconfirmed', 'formattedTime',
+      'pagesize', 'txntype', 'txnCount', 'qricon', 'qrimg', 'qrbox',
+      'paginator', 'pageplus', 'pageminus', 'listbox', 'table',
+      'range', 'chartbox', 'noconfirms', 'chart', 'pagebuttons']
  }

  initialize () {


Most of the code inside initialize seems like it should be in connect instead so that for example if you navigate from one address to another via search bar, the state is wiped and rebuilt from scratch.
hotwired/stimulus#75

In our case, because we are not re-inserting controller elements, the effect is the same. The stimulus docs are a little confusing around that point, though, so I'm just going to move everything to connect so that nobody has to wonder.

chappjc

utACK

chappjc · 2018-12-11T22:49:11Z

db/dcrpg/pgblockchain.go

@@ -711,23 +720,23 @@ func (pgb *ChainDB) HeightHashDB() (uint64, string, error) {
 }

 // Height uses the last stored height.
-func (pgb *ChainDB) Height() uint64 {


I'm surprised losing (*ChainDB).Height/Hash didn't affect more code, namely callers from other packages.

To stabilize the ChainDB API a bit, we could add some wrappers for the removed functions, but losing Height and Hash might be a good thing since it wasn't completely clear when to use Height vs. HeightDB, and Hash vs. HashDB. So anyway, this looks good.

chappjc · 2018-12-13T22:24:29Z

I would expect the x-axis to stay the same if possible.

method. Moves heavy lifting to data source and opens up some optimization possibilities

…ved to connect.

buck54321 · 2018-12-17T15:52:54Z

This was more involved than you would think it would be, so I want to motivate some of the changes here.

Added an appropriate amount of x padding around the data. The only way I can seem to do this with Dygraph is by adding additional dummy points at the beginning and end of the data set. This solves a couple of issues. First, the graph view will always contain full bars, where before, it always cut off in the middle of a bar. Second, this allows us to plot single points where previously charts would have been disabled for single points. Additionally, this allows an address with all transactions on the same day, but in multiple blocks, to group the transctions by day. Previously, the day grouping was disabled in this case.
Bar widths now correspond to bin size. Previously, there was an algorithm that iterated the data looking for the smallest gap, and based the bar width on that. That's clever, and sometimes might look better, but it makes it difficult to display things nicely, and I'm also fairly certain that for a bar graph, the expected behavior is that the bar width will correspond to the bin size.
When changing views, zoom ranges are "validated". The zoom is "clamped" and any pre-selected zoom button (day, week, ...) are properly applied to the new data range.
I made a few style changes that I should have put into a separate PR but didn't, so I'm hoping we can find agreement. Otherwise I'll roll them back. On the other hand, if we like the changes, I can apply the styling to /graphs, /ticketpool, ... as well. There are some additional, site-wide, graph styling changes I'd like to fiddle with, but I'll put them in a separate PR.

Another potential issue related to @chappjc's comment is that the "unspent" data returned from the api (see apiroutes.getAddressTxUnspentAmountData) is always empty if the address currently has zero balance, even if there were times when the address had a balance. Shouldn't the "Total Unspent" chart essentially show the address balance over time/block?

chappjc · 2018-12-17T17:31:38Z

Yeah, I agree re: "for a bar graph, the expected behavior is that the bar width will correspond to the bin size."

I think it's OK to make style changes along with these changes, especially if they facilitate addressing the issues the PR is meant to address, but depending on the change (is it self-contained and potentially applicable to 3.1, will it be easier to rebase with a separate commit, etc.) it might be good to have those as a separate commit in this PR. I'll leave that up to you.

buck54321 · 2018-12-17T19:13:23Z

The style changes only apply to the address chart, and are not necessary or important to back port. I'd love to leave them here.

chappjc · 2018-12-17T19:53:47Z

Cool. Works for me then.

…

On Mon, Dec 17, 2018, 1:13 PM buck54321 ***@***.*** wrote: The style changes only apply to the address chart, and are not necessary or important to back port. I'd love to leave them here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#845 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI8HSQzmxIQhecp6Cj6dLceKJJslPe7oks5u5-zUgaJpZM4Y4N6e> .

…ountflow

buck54321 · 2018-12-17T20:01:51Z

I reworked the unspent graph into a balance over time graph. It uses the same API data as the "Sent and Received" graph. Both chart datasets are processed simultaneously, which saves another API call.

chappjc · 2018-12-17T22:55:19Z

Would be awesome to get this in asap, so I'll give it another spin tonight.

chappjc · 2018-12-18T02:51:19Z

The issue with disappearing elements that I mentioned in this comment still exists for me (production build with npm run build vs watch):

chappjc

The issue in my previous comment is resolved.

This PR tests out perfectly. It's a great improvement to the address page.

chappjc · 2018-12-18T14:46:25Z

@gozart1 This is a g2g from my perspective. Wanna have another look before merge?

buck54321 · 2018-12-18T21:30:09Z

Final commit message

Front- and back-end fixes and tweaks for the address page.

Fix browser arrow navigation. Move list pagination to stimulus/ajax. Give buttons a more modern style. Store all list and chart settings as part of the url query parameters. Rework "Unspent Total" chart into "Balance" chart (balance v. time). Add padding to chart x range to improve data display.

Load chart data silently to improve page load times. Cache chart data to reduce api calls. Moved all inline scripts from address.tmpl to a stimulus controller. Skipped repeated Dygraph script loads, i.e on second+ Turbolinks visit. Skip AddessMetrics collection in favor of on-the-fly button disabling in the browser, based on downloaded data range. Relocate a few associated explorer types and data processing to db.

Expect some of these performance and style changes to be propagated to other pages, specifically charts.

buck54321 commented Nov 28, 2018

View reviewed changes

public/js/controllers/address_controller.js Outdated Show resolved Hide resolved

chappjc mentioned this pull request Dec 3, 2018

js: replace history state when address page goes to list-view #815

Closed

buck54321 changed the title ~~[WIP] Turbolinks-integrated url anchor navigation on /address~~ [WIP] Turbolinks-integrated query parameters and stimulus integration on address page Dec 3, 2018

chappjc reviewed Dec 3, 2018

View reviewed changes

public/js/controllers/address_controller.js Outdated Show resolved Hide resolved

chappjc reviewed Dec 3, 2018

View reviewed changes

public/js/controllers/address_controller.js Outdated Show resolved Hide resolved

chappjc reviewed Dec 3, 2018

View reviewed changes

gozart1 reviewed Dec 6, 2018

View reviewed changes

chappjc mentioned this pull request Dec 7, 2018

[3.1] add permalink icon on charts page #867

Closed

buck54321 force-pushed the addr-nav branch from cf0efb6 to 5033c45 Compare December 9, 2018 21:12

buck54321 mentioned this pull request Dec 9, 2018

Inline JS in template files #843

Closed

9 tasks

buck54321 force-pushed the addr-nav branch from 5033c45 to 54ed2a0 Compare December 9, 2018 22:51

chappjc requested changes Dec 10, 2018

View reviewed changes

chappjc changed the title ~~[WIP] Turbolinks-integrated query parameters and stimulus integration on address page~~ Turbolinks-integrated query parameters and stimulus integration on address page Dec 10, 2018

chappjc added enhancement frontend labels Dec 10, 2018

chappjc added this to the 4.0 milestone Dec 10, 2018

gozart1 reviewed Dec 11, 2018

View reviewed changes

buck54321 force-pushed the addr-nav branch from 54ed2a0 to 202a8fd Compare December 11, 2018 19:04

chappjc approved these changes Dec 11, 2018

View reviewed changes

chappjc mentioned this pull request Dec 14, 2018

refactoring for httptest #887

Merged

buck54321 added 11 commits December 17, 2018 09:45

Turbolinks-integrated navigation control

e4ca81a

switched to query parameters, adapted inline -> stimulus

6187ed3

Smarter query parameter sets. Speed and caching improvements.

9cbca67

moved AddressInfo and related types from explorer to dbtypes

3eb4720

Refactored AddressInfo processing from explorer.AddressPage to chainDB

9b7a468

method. Moves heavy lifting to data source and opens up some optimization possibilities

Moved address table to a template

4336bc3

Table pagination without chart reloads

e7e2475

Metrics calculated directly from chart data.

bf6918c

Full stimulus integration. Refactored address data collection.

0426d09

ChainDB mutex protected BestBlock member. stimulus initialize code mo…

530a7fe

…ved to connect.

Chart padding. zoom and bin button styling. zoom validation.

acf267a

buck54321 force-pushed the addr-nav branch from 202a8fd to acf267a Compare December 17, 2018 15:51

unspent chart changed to balance over time, using same api data as am…

9ac361b

…ountflow

chappjc approved these changes Dec 18, 2018

View reviewed changes

chappjc merged commit 49443c4 into decred:master Dec 18, 2018

This was referenced Dec 19, 2018

AddressMetrics runs too slowly #835

Closed

address page, maintain page size setting #643

Closed

buck54321 mentioned this pull request Mar 14, 2019

cache/dcrpg: per-address History/charts data cache #1136

Merged

chappjc mentioned this pull request Jun 8, 2021

migrate from turbolinks to turbo drive #1834

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turbolinks-integrated query parameters and stimulus integration on address page #845

Turbolinks-integrated query parameters and stimulus integration on address page #845

buck54321 commented Nov 28, 2018

chappjc commented Nov 29, 2018

buck54321 commented Nov 29, 2018

ZeroASIC commented Nov 29, 2018

chappjc commented Nov 29, 2018

ZeroASIC commented Nov 30, 2018 •

edited

buck54321 commented Nov 30, 2018

ZeroASIC commented Nov 30, 2018

chappjc commented Dec 3, 2018

buck54321 commented Dec 3, 2018

chappjc left a comment •

edited

chappjc commented Dec 4, 2018 •

edited

gozart1 Dec 6, 2018

buck54321 Dec 7, 2018

gozart1 Dec 8, 2018

buck54321 commented Dec 9, 2018 •

edited

chappjc left a comment

chappjc Dec 10, 2018

gozart1 Dec 11, 2018

buck54321 Dec 11, 2018

chappjc left a comment

chappjc Dec 11, 2018 •

edited

chappjc commented Dec 13, 2018

buck54321 commented Dec 17, 2018

chappjc commented Dec 17, 2018

buck54321 commented Dec 17, 2018

chappjc commented Dec 17, 2018 via email

buck54321 commented Dec 17, 2018

chappjc commented Dec 17, 2018

chappjc commented Dec 18, 2018

chappjc left a comment

chappjc commented Dec 18, 2018

buck54321 commented Dec 18, 2018

Turbolinks-integrated query parameters and stimulus integration on address page #845

Turbolinks-integrated query parameters and stimulus integration on address page #845

Conversation

buck54321 commented Nov 28, 2018

chappjc commented Nov 29, 2018

buck54321 commented Nov 29, 2018

ZeroASIC commented Nov 29, 2018

chappjc commented Nov 29, 2018

ZeroASIC commented Nov 30, 2018 • edited

buck54321 commented Nov 30, 2018

ZeroASIC commented Nov 30, 2018

chappjc commented Dec 3, 2018

buck54321 commented Dec 3, 2018

chappjc left a comment • edited

Choose a reason for hiding this comment

chappjc commented Dec 4, 2018 • edited

gozart1 Dec 6, 2018

Choose a reason for hiding this comment

buck54321 Dec 7, 2018

Choose a reason for hiding this comment

gozart1 Dec 8, 2018

Choose a reason for hiding this comment

buck54321 commented Dec 9, 2018 • edited

chappjc left a comment

Choose a reason for hiding this comment

chappjc Dec 10, 2018

Choose a reason for hiding this comment

gozart1 Dec 11, 2018

Choose a reason for hiding this comment

buck54321 Dec 11, 2018

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

chappjc Dec 11, 2018 • edited

Choose a reason for hiding this comment

chappjc commented Dec 13, 2018

buck54321 commented Dec 17, 2018

chappjc commented Dec 17, 2018

buck54321 commented Dec 17, 2018

chappjc commented Dec 17, 2018 via email

buck54321 commented Dec 17, 2018

chappjc commented Dec 17, 2018

chappjc commented Dec 18, 2018

chappjc left a comment

Choose a reason for hiding this comment

chappjc commented Dec 18, 2018

buck54321 commented Dec 18, 2018

ZeroASIC commented Nov 30, 2018 •

edited

chappjc left a comment •

edited

chappjc commented Dec 4, 2018 •

edited

buck54321 commented Dec 9, 2018 •

edited

chappjc Dec 11, 2018 •

edited