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

client: Restore “Mark all” button to 2.18 behaviour #1460

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

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Jul 29, 2023

In 2.18, when in “unread” mode, if there were no unread items, clicking the button would clear out all items and trigger a reload or refresh status of the “Load more” button, depending on whether the client was aware of more unread items on the server.

When we ported entries to React in f3b279c, we accidentally broke the “Mark all as read” button, making it keep all the items it marked in the view (a). We also did not port the code that would load more items afterwards (b).

It got further broken during porting routing to React in 405a3ec. We looked for the filter field on a wrong object (c).

We fixed (c) and attempted to fix (a) in ec544b7 but the patch just made it keep all the items it did not mark, such as those marked as read manually beforehand, instead.

This patch makes it so that the button removes all the items in the view except for the active item and loads more items afterwards, returning to behaviour similar to 2.18.

In 2.18, when in “unread” mode, if there were no unread items,
clicking the button would clear out all items and trigger a reload
or refresh status of the “Load more” button, depending on whether
the client was aware of more unread items on the server.

When we ported entries to React in f3b279c,
we accidentally broke the “Mark all as read” button,
making it keep all the items it marked in the view (a).
We also did not port the code that would load more items afterwards (b).

It got further broken during porting routing to React in 405a3ec.
We looked for the `filter` field on a wrong object (c).

We fixed (c) and attempted to fix (a) in ec544b7
but the patch just made it keep all the items it did not mark, such as those marked as read
manually beforehand, instead.

This patch makes it so that the button removes all the items in the view except for the active item
and loads more items afterwards, returning to behaviour similar to 2.18.

Since we currently do not have a sane dispatch method in EntriesPage,
we unfortunately have to trigger a virtual click on the “Load more” button.
To make that work, we also need to keep the old offsets since when we clear the entries,
there would be no way to get the offsets back in the `moreOnClick` handler.
This does not help since the offset is only calculated on pressing the button (initially undefined).
@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for selfoss canceled.

Name Link
🔨 Latest commit 4e6a3fc
🔍 Latest deploy log https://app.netlify.com/sites/selfoss/deploys/64c62749f9530400081e91cc

@jtojnar
Copy link
Member Author

jtojnar commented Jul 29, 2023

Because the entries are now cleared, we cannot use the simple hack of just clicking the “Load more” button but need a proper dispatcher to be able to pass the entries before deletion.

Unfortunately, the dispatcher appears to bring another bug:

  1. Set auto_stream_more=0 and items_perpage=2
  2. In “Unread” mode click “Mark as read” button.
  3. Click “Newest” mode.
  4. Observe that the “Load more” spinner will be stuck on spinning.

@jtojnar
Copy link
Member Author

jtojnar commented Jul 29, 2023

I also noticed the following weird behaviour but that does not appear to be a regression:

  1. In “Newest”, click “Load more”.
  2. Click “Newest” again.
  3. Last loaded page will be appended again.

This is still somewhat broken: switching filter will trigger `RESET_OFFSET`,
which will start loading more, and its spinner will get stuck.
The aborting short circuit has been introduced in 28d27f3

It can happen that the request is cancelled by another request but the second request
will use `setLoadingState` instead of `setMoreLoadingState` so `moreLoadingState` will be stuck on `LOADING`.

Not sure if it this cannot introduce a race if the same state is used in both requests.

Tried testing with {let a = new AbortController(); let f = fetch('wait.php?seconds=3', {signal: a.signal}); f.then(r => console.log(r, a)); setTimeout(() => {a.abort(); f.then((r) => console.log(r, a.signal))}, 100);}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant