Replace page numbers with ID based selection for trace indexes #4089
Conversation
There was a problem hiding this comment.
Overall I'm happy with the idea of moving to cursor-based pagination. I'd like to do the same for all of our existing offset-based pagination, since it all has the same problems as here (degraded performance for increasing pagenumbers, highly unstable urls, etc).
In addition to the line-by-line comments, I wonder if rather than trying to get the logic and edge cases right (and making all the logic etc resuable across controllers) I wonder if we are better off finding a gem that does what we want. There's a few different ones that I've found, such as rails_cursor_pagination, pagy_cursor, etc
| <li class="page-item"> | ||
| <%= link_to t(".newer"), params.merge(:page => page - 1), :class => "page-link" %> | ||
| <%= link_to t(".newer"), params.merge(:min_id => traces.first.id + 1, :max_id => nil), :class => "page-link" %> |
There was a problem hiding this comment.
The logic here isn't working quite right. If I go to the final page of traces on my system (i.e. press "older" a few times) then I get a link to newer traces e.g. "min_id=3". But that link shows me the first page of traces, not the second-to-last.
Since traces are reverse sorted, traces.where(id >= 3).order_by(id desc).limit(25) returns traces that are max(id) to max(id)-25 e.g. 1003 to 978, instead of 27 to 3 which is what was intended.
app/views/traces/index.html.erb
Outdated
| </li> | ||
| <!-- my traces --> | ||
| <li class="nav-item"> | ||
| <%= link_to t(".my_traces"), { :action => "mine", :page => nil }, { :class => "nav-link active" } %> | ||
| <%= link_to t(".my_traces"), { :action => "mine", :max_id => nil }, { :class => "nav-link active" } %> |
There was a problem hiding this comment.
I don't think any of these :max_id => nil are needed, since the link is being built from scratch (other links are built from merging with existing params elsewhere).
Or if I've misunderstood, and they are necessary, then I think a :min_id => nil would be needed too.
app/controllers/traces_controller.rb
Outdated
| @@ -54,14 +54,16 @@ def index | |||
|
|
|||
| @traces = @traces.tagged(params[:tag]) if params[:tag] | |||
|
|
|||
| @params = params.permit(:display_name, :tag) | |||
| @params = params.permit(:display_name, :tag, :min_id, :max_id) | |||
There was a problem hiding this comment.
Most implementations I've seen use "before" and "after" as the parameters. I'm not sure why, but maybe it makes it easier when reasoning about id sort order? (see other comment)
There was a problem hiding this comment.
I based max_id on what we were already doing doing for changesets in the history view but I wasn't especially keen on it.
|
I've pushed a new version that I think addresses all your review comments. I did look at the possibility of using a gem but I'm not convinced there's anything that's useful for various reasons. In many ways pagy_cursor looks excellent, especially since it's an extension to pagy which provides nice view helpers and would be good to replace our existing paging code for places where we do real paging. The problem is that it doesn't really integrate with pagy in any meaningful way and you can't use the pagy navigation helpers as Uysim/pagy-cursor#21 makes clear. It's not entirely their fault as pagy would need changes to allow it to support cursor based paging but there doesn't seem to be any effort to properly integrate them. As to rails_cursor_pagination there is the issue of the rather ugly cursors but we could live with that - you could even argue it's an advantage. There's a killer bug at the moment though (xing/rails_cursor_pagination#132) in that even if you disable totals it still does the query when working our whether next/previous links are needed! |
This allows us to avoid horribly inefficient queries when people try and fetch ever large page numbers for the trace list forcing us to read ever large number of records.
It also means the URLs remain stable and will always show more or less the same traces, modulo any changes when traces are deleted, whereas page numbers change constantly over time.