<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<p dir="auto">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).</p>
<p dir="auto">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 <a href="https://github.com/xing/rails_cursor_pagination">rails_cursor_pagination</a>, <a href="https://github.com/Uysim/pagy-cursor">pagy_cursor</a>, <a href="https://rubygems.org/search?query=name%3A+cursor++++updated%3A+%3E2021" rel="nofollow">etc</a></p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4089#discussion_r1268368569">app/views/traces/_trace_paging_nav.html.erb</a>:</p>
<pre style='color:#555'>> <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" %>
</pre>
<p dir="auto">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.</p>
<p dir="auto">Since traces are reverse sorted, <code class="notranslate">traces.where(id >= 3).order_by(id desc).limit(25)</code> returns traces that are max(id) to max(id)-25 e.g. <code class="notranslate">1003</code> to <code class="notranslate">978</code>, instead of <code class="notranslate">27</code> to <code class="notranslate">3</code> which is what was intended.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4089#discussion_r1268371748">app/views/traces/_trace_paging_nav.html.erb</a>:</p>
<pre style='color:#555'>> </li>
<% else %>
<li class="page-item disabled">
<span class="page-link"><%= t(".newer") %></span>
</li>
<% end %>
- <li class="page-item disabled">
- <span class="page-link"><%= t(".showing_page", :page => page) %></span>
</pre>
<p dir="auto">Unused translation can be removed from <code class="notranslate">en.yml</code></p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4089#discussion_r1268373046">app/views/traces/index.html.erb</a>:</p>
<pre style='color:#555'>> </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" } %>
</pre>
<p dir="auto">I don't think any of these <code class="notranslate">:max_id => nil</code> are needed, since the link is being built from scratch (other links are built from merging with existing params elsewhere).</p>
<p dir="auto">Or if I've misunderstood, and they are necessary, then I think a <code class="notranslate">:min_id => nil</code> would be needed too.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4089#discussion_r1268377339">test/controllers/traces_controller_test.rb</a>:</p>
<pre style='color:#555'>> @@ -8,51 +8,27 @@ def test_routes
{ :path => "/traces", :method => :get },
{ :controller => "traces", :action => "index" }
)
- assert_routing(
- { :path => "/traces/page/1", :method => :get },
- { :controller => "traces", :action => "index", :page => "1" }
- )
</pre>
<p dir="auto">We should keep some some redirect tests to make sure the redirects aren't accidentally refactored away in future.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4089#discussion_r1268380411">app/controllers/traces_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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)
</pre>
<p dir="auto">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)</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4089#pullrequestreview-1537559541">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLM6GAP5NUM2W42SEOLXRAJM7ANCNFSM6AAAAAA2H4NI4Q">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLNUQUXKWR3S2FAPBODXRAJM7A5CNFSM6AAAAAA2H4NI4SWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTS3UVF7K.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/4089/review/1537559541</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/4089#pullrequestreview-1537559541",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4089#pullrequestreview-1537559541",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>