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

Convert note pagination to newer/older note pages #4532

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented Feb 23, 2024

image

Most of the changes here are because of potentially unstable ordering by update time. Previously implemented pagination code sorted things by ids that are not changing. Here you can get occasions like having a link to the next page that was supposed to contain one note when it was generated, but before you click it someone comments that note, now it's no longer on that page and the page is empty when opened. How do you get a link to the previous page? Previously you'd have after=(id of first note of the page) but now there are no notes.

I started doing this after I noticed strings for changeset_paging_nav here. Do we use them for paging changesets? No, but we use them for notes.

@tomhughes
Copy link
Member

There's a number of uncontroversial changes here which aren't directly related to this PR and could be handled separately but having now spent about half an hour staring at this the core of it concerns me - it seems to be introducing an extraordinary level of complexity to me and I don't claim to understand it all yet at all.

I wonder if we're missing a trick here - although this view is a list of notes what it's really doing is listing notes in order of their more recent comment so is there a way we can use the comment ID values, which are nice and stable, as the cursor?

@AntonKhorev
Copy link
Collaborator Author

AntonKhorev commented Mar 12, 2024

is there a way we can use the comment ID values, which are nice and stable, as the cursor?

Something like this?

  1. don't do Note: Add body & author #4481, otherwise not all notes have a comment
  2. mark the last note comment / unmark previously last comment when updating the note
  3. get marked comments sorted by ids

Not sure about table joins if we want to replicate the existing behavior where user's notes are all notes acted upon by the user. We get a comment then join it with a note, then join it again with comments to see if one of them is the user's. So probably we'll sill have to get notes sorted by update dates.

The ordering of notes is still going to be unstable even with this approach. What it allows is to not have to use fallback timestamps and use comment ids instead.

@AntonKhorev AntonKhorev force-pushed the note-pager branch 2 times, most recently from a2a3ac6 to cfc6fc0 Compare March 21, 2024 01:46
@AntonKhorev AntonKhorev mentioned this pull request Mar 21, 2024
@AntonKhorev AntonKhorev marked this pull request as draft March 22, 2024 16:57
@mmd-osm mmd-osm added the notes Related to the notes feature label Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes Related to the notes feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants