From 611cd8381aa4dfbd7ef6b85ab4276d7ccdada7ab Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sat, 29 Jul 2023 21:31:03 +0200 Subject: [PATCH 1/7] =?UTF-8?q?client:=20Restore=20=E2=80=9CMark=20all?= =?UTF-8?q?=E2=80=9D=20button=20to=202.18=20behaviour?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 f3b279c808e8a5d7e3ab5c8254bdda83c094b50e, 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 405a3ec2cf49e3dc17421b997930c5b1faebd2fc. We looked for the `filter` field on a wrong object (c). We fixed (c) and attempted to fix (a) in ec544b7ae066050dcdb43e362f38854d4fdb7bea 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. --- client/js/templates/EntriesPage.jsx | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/client/js/templates/EntriesPage.jsx b/client/js/templates/EntriesPage.jsx index 85cade38a7..3b1044f66b 100644 --- a/client/js/templates/EntriesPage.jsx +++ b/client/js/templates/EntriesPage.jsx @@ -329,6 +329,14 @@ export function EntriesPage({ const moreOnClick = useCallback( (event) => { event.preventDefault(); + + if (entries.length === 0) { + // If there are no entries (e.g. after clicking “Mark as read” button in “Unread” filter mode), + // keep the old offsets. This can also happen when switching to an empty source/tag but + // then `hasMore` should be false so the offset from wrong source/tag are moot. + return; + } + const lastEntry = entries[entries.length - 1]; // Calculate offset. @@ -753,8 +761,8 @@ export default class StateHolder extends React.Component { this.setExpandedEntries({}); this.props.setNavExpanded(false); - if (ids.length !== 0 && this.props.match.params.filter === FilterType.UNREAD) { - markedEntries = markedEntries.filter(({ id }) => !ids.includes(id)); + if (this.props.match.params.filter === FilterType.UNREAD) { + markedEntries = markedEntries.filter(({ id }) => parseInt(this.props.match.params.id ?? null, 10) === id); } this.setLoadingState(LoadingState.LOADING); @@ -790,6 +798,11 @@ export default class StateHolder extends React.Component { itemsRequests.markAll(ids).then(() => { this.setLoadingState(LoadingState.SUCCESS); + + requestAnimationFrame(() => { + // attempt to load more + document.querySelector('.stream-more')?.click(); + }); }).catch((error) => { selfoss.handleAjaxError(error).then(() => { const statuses = ids.map((id) => ({ From b7faafb61492cc6175b43320957df4e9ef0fe56d Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sun, 30 Jul 2023 00:50:51 +0200 Subject: [PATCH 2/7] client: Revert entries.length check This does not help since the offset is only calculated on pressing the button (initially undefined). --- client/js/templates/EntriesPage.jsx | 8 -------- 1 file changed, 8 deletions(-) diff --git a/client/js/templates/EntriesPage.jsx b/client/js/templates/EntriesPage.jsx index 3b1044f66b..2eca6956cf 100644 --- a/client/js/templates/EntriesPage.jsx +++ b/client/js/templates/EntriesPage.jsx @@ -329,14 +329,6 @@ export function EntriesPage({ const moreOnClick = useCallback( (event) => { event.preventDefault(); - - if (entries.length === 0) { - // If there are no entries (e.g. after clicking “Mark as read” button in “Unread” filter mode), - // keep the old offsets. This can also happen when switching to an empty source/tag but - // then `hasMore` should be false so the offset from wrong source/tag are moot. - return; - } - const lastEntry = entries[entries.length - 1]; // Calculate offset. From cd7e0197e2438293cef52455b70f5c40e7a7337b Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sun, 30 Jul 2023 01:29:45 +0200 Subject: [PATCH 3/7] client: Try to add dispatch This is still somewhat broken: switching filter will trigger `RESET_OFFSET`, which will start loading more, and its spinner will get stuck. --- client/js/templates/EntriesPage.jsx | 83 ++++++++++++++++++++++++----- 1 file changed, 70 insertions(+), 13 deletions(-) diff --git a/client/js/templates/EntriesPage.jsx b/client/js/templates/EntriesPage.jsx index 2eca6956cf..d24e4500cb 100644 --- a/client/js/templates/EntriesPage.jsx +++ b/client/js/templates/EntriesPage.jsx @@ -4,6 +4,7 @@ import React, { useEffect, useMemo, useState, + useReducer, } from 'react'; import PropTypes from 'prop-types'; import { Link, useLocation, useParams } from 'react-router-dom'; @@ -25,6 +26,38 @@ import { useShouldReload } from '../helpers/hooks'; import { forceReload, makeEntriesLinkLocation } from '../helpers/uri'; import { HttpError } from '../errors'; +const LOAD_MORE = 'load-more'; +const RESET_OFFSET = 'reset-offset'; + +function entriesReducer( + state, + action, +) { + switch (action.type) { + case LOAD_MORE: + const { entries } = action.payload; + const lastEntry = entries[entries.length - 1]; + + return { + ...state, + // Calculate offset. + fromDatetime: lastEntry ? lastEntry.datetime : undefined, + fromId: lastEntry ? lastEntry.id : undefined, + }; + + case RESET_OFFSET: + return { + ...state, + // Calculate offset. + fromDatetime: undefined, + fromId: undefined, + }; + + default: + throw new Error('Unknown action.'); + } +} + function reloadList({ fetchParams, abortController, @@ -195,6 +228,7 @@ export function EntriesPage({ reload, setGlobalUnreadCount, unreadItemsCount, + setDispatchEntries, }) { const allowedToUpdate = useAllowedToUpdate(); const allowedToWrite = useAllowedToWrite(); @@ -212,14 +246,28 @@ export function EntriesPage({ const currentTag = params.category?.startsWith('tag-') ? params.category.replace(/^tag-/, '') : null; const currentSource = params.category?.startsWith('source-') ? parseInt(params.category.replace(/^source-/, ''), 10) : null; - // The offsets for pagination. - // Clear them when URL changes, except for when only id changes since that happens when reading. - const [fromDatetime, setFromDatetime] = useStateWithDeps( - undefined, - [params.filter, currentTag, currentSource, searchText] + const [entriesState, dispatchEntries] = useReducer( + entriesReducer, + { + // The offsets for pagination. + // Clear them when URL changes, except for when only id changes since that happens when reading. + fromDatetime: undefined, + fromId: undefined, + } ); - const [fromId, setFromId] = useStateWithDeps( - undefined, + + // Propagate the dispatcher to stateful class component. + setDispatchEntries(dispatchEntries); + + const { + fromDatetime, + fromId, + } = entriesState; + + useEffect( + () => dispatchEntries({ + type: RESET_OFFSET, + }), [params.filter, currentTag, currentSource, searchText] ); @@ -329,13 +377,14 @@ export function EntriesPage({ const moreOnClick = useCallback( (event) => { event.preventDefault(); - const lastEntry = entries[entries.length - 1]; - - // Calculate offset. - setFromDatetime(lastEntry ? lastEntry.datetime : undefined); - setFromId(lastEntry ? lastEntry.id : undefined); + dispatchEntries({ + type: LOAD_MORE, + payload: { + entries, + }, + }); }, - [entries, setFromDatetime, setFromId] + [entries] ); // Current time for calculating relative dates in items. @@ -445,6 +494,7 @@ EntriesPage.propTypes = { reload: PropTypes.func.isRequired, setGlobalUnreadCount: PropTypes.func.isRequired, unreadItemsCount: PropTypes.number.isRequired, + setDispatchEntries: PropTypes.func.isRequired, }; const initialState = { @@ -461,6 +511,8 @@ const initialState = { }; export default class StateHolder extends React.Component { + dispatchEntries = () => {}; + constructor(props) { super(props); this.state = initialState; @@ -1081,6 +1133,10 @@ export default class StateHolder extends React.Component { } render() { + const setDispatchEntries = (dispatchEntries) => { + this.dispatchEntries = dispatchEntries; + }; + return ( ); } From 3bb2fb498de89c7f88c4b33bc0a4fee438f5f9c6 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sun, 30 Jul 2023 01:30:00 +0200 Subject: [PATCH 4/7] client: switch mark all to dispatch --- client/js/templates/EntriesPage.jsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/client/js/templates/EntriesPage.jsx b/client/js/templates/EntriesPage.jsx index d24e4500cb..4c44859522 100644 --- a/client/js/templates/EntriesPage.jsx +++ b/client/js/templates/EntriesPage.jsx @@ -843,9 +843,11 @@ export default class StateHolder extends React.Component { itemsRequests.markAll(ids).then(() => { this.setLoadingState(LoadingState.SUCCESS); - requestAnimationFrame(() => { - // attempt to load more - document.querySelector('.stream-more')?.click(); + this.dispatchEntries({ + type: LOAD_MORE, + payload: { + entries: oldEntries, + }, }); }).catch((error) => { selfoss.handleAjaxError(error).then(() => { From 3d77e1eecdffc9f29da1ad4ff6f403c502e80706 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sun, 30 Jul 2023 01:31:22 +0200 Subject: [PATCH 5/7] client: Load more on marking all as read only in unread mode --- client/js/templates/EntriesPage.jsx | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/client/js/templates/EntriesPage.jsx b/client/js/templates/EntriesPage.jsx index 4c44859522..29a1e566aa 100644 --- a/client/js/templates/EntriesPage.jsx +++ b/client/js/templates/EntriesPage.jsx @@ -843,12 +843,15 @@ export default class StateHolder extends React.Component { itemsRequests.markAll(ids).then(() => { this.setLoadingState(LoadingState.SUCCESS); - this.dispatchEntries({ - type: LOAD_MORE, - payload: { - entries: oldEntries, - }, - }); + if (this.props.match.params.filter === FilterType.UNREAD) { + // We cleared out the entries page, load more. + this.dispatchEntries({ + type: LOAD_MORE, + payload: { + entries: oldEntries, + }, + }); + } }).catch((error) => { selfoss.handleAjaxError(error).then(() => { const statuses = ids.map((id) => ({ From 2aef72e9116bb2479a83111ac220668d07ef14bc Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sun, 30 Jul 2023 04:59:52 +0200 Subject: [PATCH 6/7] client: Restore spinner even when the event has been cancelled The aborting short circuit has been introduced in 28d27f31c7dd0441d6088fbf9d4ed80fa6dbf237 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);} --- client/js/templates/EntriesPage.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/js/templates/EntriesPage.jsx b/client/js/templates/EntriesPage.jsx index 29a1e566aa..d19697565e 100644 --- a/client/js/templates/EntriesPage.jsx +++ b/client/js/templates/EntriesPage.jsx @@ -111,13 +111,13 @@ function reloadList({ setLoadingState(LoadingState.LOADING); return reloader(fetchParams, abortController).then(({ entries, hasMore }) => { + setLoadingState(LoadingState.SUCCESS); + selfoss.entriesPage.setHasMore(hasMore); + if (abortController.signal.aborted) { return; } - setLoadingState(LoadingState.SUCCESS); - selfoss.entriesPage.setHasMore(hasMore); - if (append) { selfoss.entriesPage.appendEntries(entries); } else { From 4e6a3fc47ab0fdfaed695f24b08861cb18040cc9 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Sun, 30 Jul 2023 10:36:27 +0200 Subject: [PATCH 7/7] client: Add more debugging --- client/js/templates/EntriesPage.jsx | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/client/js/templates/EntriesPage.jsx b/client/js/templates/EntriesPage.jsx index d19697565e..bc05fdd1fe 100644 --- a/client/js/templates/EntriesPage.jsx +++ b/client/js/templates/EntriesPage.jsx @@ -33,6 +33,7 @@ function entriesReducer( state, action, ) { + console.log(state, action) switch (action.type) { case LOAD_MORE: const { entries } = action.payload; @@ -109,14 +110,25 @@ function reloadList({ selfoss.entriesPage.setSelectedEntry(null); } + const reloader_ = reloader; + + reloader = (...args) => { + const r = reloader_(...args); + console.trace('reloader', r); + return r; + } + setLoadingState(LoadingState.LOADING); return reloader(fetchParams, abortController).then(({ entries, hasMore }) => { setLoadingState(LoadingState.SUCCESS); - selfoss.entriesPage.setHasMore(hasMore); + console.trace('reloader-then', abortController.signal, abortController.signal.aborted); if (abortController.signal.aborted) { return; } + console.log('after') + + selfoss.entriesPage.setHasMore(hasMore); if (append) { selfoss.entriesPage.appendEntries(entries); @@ -284,7 +296,11 @@ export function EntriesPage({ return navSourcesExpanded; }, [params.filter, currentTag, currentSource, searchText]); - const [moreLoadingState, setMoreLoadingState] = useState(LoadingState.INITIAL); + const [moreLoadingState, setMoreLoadingState_] = useState(LoadingState.INITIAL); + const setMoreLoadingState = (...args) => { + console.trace('moreState', ...args); + setMoreLoadingState_(...args); + }; // Perform the scheduled reload. useEffect(() => {