openstreetmap / openstreetmap-website

The Rails application that powers OpenStreetMap
https://www.openstreetmap.org/
GNU General Public License v2.0
2.18k stars 914 forks source link

Add Sorting with Turbo Pagination and Extend to Sortable Columns #5259

Open kcne opened 1 day ago

kcne commented 1 day ago

Problem

Currently, we use a custom PaginationMethods module, which sorts and filters data using IDs. Sorting by other attributes (e.g., created_at or updated_at) does not work correctly, and implementing sorting with these attributes would require changes to PaginationMethods. Additionally, the Notes page does not currently use Turbo, but refactoring it to support sorting with pagination would allow seamless integration of this feature.

Description

PaginationMethods could be modified module to handle sorting on attributes other than IDs, integrated with Turbo for pagination. This would allow us to add sorting to tables across the site. As an example, this feature could later be extended to the Notes page, where adding Turbo pagination would enhance the user experience with sortable columns and icons next to headers.

I am eager to do some research and explore the best approach for this. Would appreciate any suggestions or ideas on how to proceed.

Screenshots

On UI end integrating this into tables could look something like this(we would just add the sorting icons):

Screenshot 2024-10-13 at 16 21 36
tomhughes commented 1 day ago

Did you close this because you realised the problem with it? That pagination needs any ordering to be on a unique index so that the cursor positioning can work...

kcne commented 12 hours ago

Tom, I want to dig a bit deeper into this to explore limitations and what are some solid ways to approach this, after which I will reopen this again. I've read few blog posts where people use Turbo with Stimulus but I'm not sure if that would fit the bill for our use case.

tomhughes commented 12 hours ago

Just to be clear my concerns are not about how the UI works, it's about the database queries being performed on the backend and whether they are safe/efficient.

kcne commented 8 hours ago

You're absolutely right about the challenges of using cursor-based pagination when sorting by non-unique columns. Given that cursor pagination works best with unique or sequential fields like id or created_at, sorting by something like name or status would cause issues.

One solution that I think could work without major disruption is to implement combined sorting. We can sort by the non-unique column (e.g., name), but also include a secondary sort by a unique field like id to maintain stable pagination. For example:

ORDER BY name ASC, id ASC

This way, cursor-based pagination would still function properly, while allowing some flexibility in sorting.

There are other alternatives we could consider, though they would require significantly more changes:

  1. Offset-based Pagination: This would allow sorting by any column but might introduce performance issues on larger datasets.

  2. Hybrid Approach: We could use cursor-based pagination for sortable fields like id and timestamps, but switch to offset-based pagination for other columns. However, this would require a more complex implementation and might not be worth the effort for now.

I think the combined sorting approach strikes a good balance between keeping the current system efficient while allowing sorting by other fields. We can explore the other options later if necessary, but they seem like they would require a lot more changes for something we’re not sure is worth the hassle at this stage.

Looking forward to your thoughts!

tomhughes commented 8 hours ago

It's only efficient if there's also an index on the columns in question and there's a limit to how many of those we want to add, and you also need uniqueness though that can be achieved by appending id to any other cursor field(s) to create a composite cursor.

The whole point of moving to cursor based pagination is to get away from offset based pagination so we definitely don't want to go back to that,