Disable mark-as-read while a sync or reload is in progress
useFeeds.js had no concept of a sync/reload being in flight, so two passive mark-as-read paths could race a wholesale fetchData()/sync() list replacement: - handleIntersection() (list-view scroll marking) - markCurrentArticleRead() (article-view display/paging marking) Both are now gated by a refcount (activeSyncCount/beginSync/endSync/ isSyncing), mirroring the existing pendingReadCounts pattern for the same class of overlapping-async problem. A refcount rather than a boolean because sync() nests fetchData() inside it, and fetchData() can also run independently while a sync() elsewhere is still in flight. Reads suppressed during a sync/reload aren't dropped: their ids are queued and flushed (marked read, and swept out of the list) once every in-flight fetchData()/sync() call has finished. The explicit "Mark all read" action is left ungated, since it's a deliberate, already-confirmed action rather than a passive side effect. 87/87 frontend tests passing.
This commit is contained in:
@@ -12,7 +12,7 @@ class FakeIntersectionObserver {
|
|||||||
vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver)
|
vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver)
|
||||||
|
|
||||||
describe('useFeeds', () => {
|
describe('useFeeds', () => {
|
||||||
const { feeds, allItems, feedFilter, feedTitles, feedUnreadCounts, unreadCount, displayedFeedUnreadCounts, refreshUnreadDisplay, setFeedFilter, showMessage, message, viewMode, fetchData, sync, getReadable, setInitialLoad, handleIntersection, markAllRead, markRead, markCurrentArticleRead, setupIntersectionObserver } = useFeeds()
|
const { feeds, allItems, feedFilter, feedTitles, feedUnreadCounts, unreadCount, displayedFeedUnreadCounts, refreshUnreadDisplay, setFeedFilter, showMessage, message, viewMode, currentIndex, fetchData, sync, getReadable, setInitialLoad, handleIntersection, markAllRead, markRead, markCurrentArticleRead, setupIntersectionObserver } = useFeeds()
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
localStorage.setItem('user-token', 'test-token')
|
localStorage.setItem('user-token', 'test-token')
|
||||||
@@ -177,6 +177,195 @@ describe('useFeeds', () => {
|
|||||||
setInitialLoad(false)
|
setInitialLoad(false)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
describe('mark-as-read suppressed while syncing', () => {
|
||||||
|
it('queues list-view scroll marking while a reload (fetchData) is in flight, and flushes it automatically once it finishes', async () => {
|
||||||
|
feeds.value = [{ id: 701, title: 'First' }]
|
||||||
|
setInitialLoad(true)
|
||||||
|
// No .observe nodes exist here, so this deterministically clears
|
||||||
|
// skipNextObservation rather than relying on whatever an earlier test
|
||||||
|
// left it as.
|
||||||
|
setupIntersectionObserver()
|
||||||
|
axios.put.mockResolvedValue({ status: 200 })
|
||||||
|
|
||||||
|
let resolveGet
|
||||||
|
axios.get.mockReturnValueOnce(new Promise(resolve => { resolveGet = resolve }))
|
||||||
|
const fetchPromise = fetchData()
|
||||||
|
|
||||||
|
await handleIntersection([
|
||||||
|
{ isIntersecting: false, boundingClientRect: { y: -10 }, target: { id: '0' } },
|
||||||
|
])
|
||||||
|
expect(axios.put).not.toHaveBeenCalled()
|
||||||
|
// Queued, not dropped — untouched while the reload is in flight, unlike
|
||||||
|
// a normal scroll-past which removes it immediately.
|
||||||
|
expect(feeds.value.map(f => f.id)).toEqual([701])
|
||||||
|
|
||||||
|
// The refetch's response still lists it (its read never committed) —
|
||||||
|
// the flush that follows must find and remove it by id.
|
||||||
|
resolveGet({
|
||||||
|
data: {
|
||||||
|
feeds: [{
|
||||||
|
title: 'Feed A',
|
||||||
|
items: [{ id: 701, title: 'First', content: '', url: 'https://example.test/701', timestamp: '2026-01-01 10:00:00' }],
|
||||||
|
}],
|
||||||
|
},
|
||||||
|
})
|
||||||
|
await fetchPromise
|
||||||
|
|
||||||
|
// Flushed automatically once the reload finished — no second scroll needed.
|
||||||
|
expect(axios.put).toHaveBeenCalledWith('/api/v1/article/read/701', null, expect.anything())
|
||||||
|
expect(feeds.value).toEqual([])
|
||||||
|
|
||||||
|
setInitialLoad(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('queues list-view scroll marking for the whole duration of sync(), and flushes it once sync finishes', async () => {
|
||||||
|
feeds.value = [{ id: 702, title: 'First' }]
|
||||||
|
setInitialLoad(true)
|
||||||
|
setupIntersectionObserver() // deterministically clears skipNextObservation — see the previous test
|
||||||
|
axios.put.mockResolvedValue({ status: 200 })
|
||||||
|
|
||||||
|
let resolvePost
|
||||||
|
axios.post.mockReturnValueOnce(new Promise(resolve => { resolvePost = resolve }))
|
||||||
|
const syncPromise = sync(true)
|
||||||
|
|
||||||
|
// Still mid network round-trip to /api/v1/article/sync — fetchData()
|
||||||
|
// hasn't even started yet, but scroll-marking must already be queued.
|
||||||
|
await handleIntersection([
|
||||||
|
{ isIntersecting: false, boundingClientRect: { y: -10 }, target: { id: '0' } },
|
||||||
|
])
|
||||||
|
expect(axios.put).not.toHaveBeenCalled()
|
||||||
|
expect(feeds.value.map(f => f.id)).toEqual([702])
|
||||||
|
|
||||||
|
axios.get.mockResolvedValueOnce({ data: { feeds: [] } })
|
||||||
|
resolvePost({ status: 200 })
|
||||||
|
await syncPromise
|
||||||
|
|
||||||
|
expect(axios.put).toHaveBeenCalledWith('/api/v1/article/read/702', null, expect.anything())
|
||||||
|
expect(feeds.value).toEqual([])
|
||||||
|
|
||||||
|
setInitialLoad(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('queues article-view display marking while a reload is in flight, and flushes it in place once the reload finishes', async () => {
|
||||||
|
viewMode.value = 'article' // still on screen when the flush runs — must not be removed out from under it
|
||||||
|
feeds.value = [{ id: 703, title: 'First' }]
|
||||||
|
axios.put.mockResolvedValue({ status: 200 })
|
||||||
|
|
||||||
|
let resolveGet
|
||||||
|
axios.get.mockReturnValueOnce(new Promise(resolve => { resolveGet = resolve }))
|
||||||
|
const fetchPromise = fetchData()
|
||||||
|
|
||||||
|
markCurrentArticleRead()
|
||||||
|
expect(axios.put).not.toHaveBeenCalled()
|
||||||
|
expect(feeds.value[0].read).toBeFalsy()
|
||||||
|
|
||||||
|
// The refetch's response still lists it (its read never committed), same
|
||||||
|
// id, now at whatever index it re-sorts to — the flush must find it by
|
||||||
|
// id, not by the currentIndex it was queued under.
|
||||||
|
resolveGet({
|
||||||
|
data: {
|
||||||
|
feeds: [{
|
||||||
|
title: 'Feed A',
|
||||||
|
items: [{ id: 703, title: 'First', content: '', url: 'https://example.test/703', timestamp: '2026-01-01 10:00:00' }],
|
||||||
|
}],
|
||||||
|
},
|
||||||
|
})
|
||||||
|
await fetchPromise
|
||||||
|
|
||||||
|
// Flushed automatically once the reload finished — no second call needed.
|
||||||
|
expect(axios.put).toHaveBeenCalledWith('/api/v1/article/read/703', null, expect.anything())
|
||||||
|
// Still in article view — marked in place, not removed (removing it here
|
||||||
|
// would yank whatever's on screen out from under currentIndex; the same
|
||||||
|
// invariant markCurrentArticleRead() already keeps while paging).
|
||||||
|
expect(feeds.value.find(f => f.id === 703)).toMatchObject({ read: true })
|
||||||
|
})
|
||||||
|
|
||||||
|
it('drops a queued article-view read once flushed, if the user already left article view before the reload finished', async () => {
|
||||||
|
feeds.value = [{ id: 707, title: 'First' }]
|
||||||
|
viewMode.value = 'article'
|
||||||
|
axios.put.mockResolvedValue({ status: 200 })
|
||||||
|
|
||||||
|
let resolveGet
|
||||||
|
axios.get.mockReturnValueOnce(new Promise(resolve => { resolveGet = resolve }))
|
||||||
|
const fetchPromise = fetchData()
|
||||||
|
|
||||||
|
markCurrentArticleRead() // queued: isSyncing() is true
|
||||||
|
// The user backs out to list view before the reload finishes —
|
||||||
|
// leaveArticleView()'s dropReadArticles() runs here, but this article
|
||||||
|
// isn't flagged read yet (it's only queued), so it survives that pass.
|
||||||
|
viewMode.value = 'list'
|
||||||
|
expect(feeds.value.map(f => f.id)).toEqual([707])
|
||||||
|
|
||||||
|
resolveGet({ data: { feeds: [] } })
|
||||||
|
await fetchPromise
|
||||||
|
|
||||||
|
// Now in list view with no further transition to rely on — the flush
|
||||||
|
// must sweep it out itself instead of leaving it stuck looking unread.
|
||||||
|
expect(axios.put).toHaveBeenCalledWith('/api/v1/article/read/707', null, expect.anything())
|
||||||
|
expect(feeds.value).toEqual([])
|
||||||
|
})
|
||||||
|
|
||||||
|
it('queues every article paged past during a sync, flushing all of them once it ends', async () => {
|
||||||
|
feeds.value = [
|
||||||
|
{ id: 704, title: 'First' },
|
||||||
|
{ id: 705, title: 'Second' },
|
||||||
|
]
|
||||||
|
axios.put.mockResolvedValue({ status: 200 })
|
||||||
|
|
||||||
|
let resolvePost
|
||||||
|
axios.post.mockReturnValueOnce(new Promise(resolve => { resolvePost = resolve }))
|
||||||
|
const syncPromise = sync(true)
|
||||||
|
|
||||||
|
markCurrentArticleRead() // pages onto article 704
|
||||||
|
currentIndex.value = 1
|
||||||
|
markCurrentArticleRead() // pages onto article 705
|
||||||
|
expect(axios.put).not.toHaveBeenCalled()
|
||||||
|
|
||||||
|
axios.get.mockResolvedValueOnce({ data: { feeds: [] } })
|
||||||
|
resolvePost({ status: 200 })
|
||||||
|
await syncPromise
|
||||||
|
|
||||||
|
expect(axios.put).toHaveBeenCalledWith('/api/v1/article/read/704', null, expect.anything())
|
||||||
|
expect(axios.put).toHaveBeenCalledWith('/api/v1/article/read/705', null, expect.anything())
|
||||||
|
|
||||||
|
currentIndex.value = 0
|
||||||
|
})
|
||||||
|
|
||||||
|
it('does not reopen the mark-as-read window when an independent fetchData() resolves while a sync() is still in flight', async () => {
|
||||||
|
feeds.value = [{ id: 706, title: 'First' }]
|
||||||
|
setInitialLoad(true)
|
||||||
|
setupIntersectionObserver() // deterministically clears skipNextObservation — see above
|
||||||
|
axios.put.mockResolvedValue({ status: 200 })
|
||||||
|
|
||||||
|
// sync() starts and is left mid network round-trip.
|
||||||
|
let resolvePost
|
||||||
|
axios.post.mockReturnValueOnce(new Promise(resolve => { resolvePost = resolve }))
|
||||||
|
const syncPromise = sync(true)
|
||||||
|
|
||||||
|
// An independent fetchData() call (e.g. a separate reload) runs to
|
||||||
|
// completion while sync()'s POST is still pending.
|
||||||
|
axios.get.mockResolvedValueOnce({ data: { feeds: [] } })
|
||||||
|
await fetchData()
|
||||||
|
|
||||||
|
// sync() itself hasn't finished — scroll-marking must still be
|
||||||
|
// suppressed, not reopened by the independent call settling first.
|
||||||
|
feeds.value = [{ id: 706, title: 'First' }]
|
||||||
|
await handleIntersection([
|
||||||
|
{ isIntersecting: false, boundingClientRect: { y: -10 }, target: { id: '0' } },
|
||||||
|
])
|
||||||
|
expect(axios.put).not.toHaveBeenCalled()
|
||||||
|
|
||||||
|
axios.get.mockResolvedValueOnce({ data: { feeds: [] } })
|
||||||
|
resolvePost({ status: 200 })
|
||||||
|
await syncPromise
|
||||||
|
|
||||||
|
// Only now, with sync() truly finished, does the queued entry flush.
|
||||||
|
expect(axios.put).toHaveBeenCalledWith('/api/v1/article/read/706', null, expect.anything())
|
||||||
|
|
||||||
|
setInitialLoad(false)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
describe('markRead resilience', () => {
|
describe('markRead resilience', () => {
|
||||||
// Mirrors useFeeds.js's MARK_READ_RETRY_DELAYS_MS (not exported). Kept in
|
// Mirrors useFeeds.js's MARK_READ_RETRY_DELAYS_MS (not exported). Kept in
|
||||||
// sync via this comment: if that backoff changes, update this too — too
|
// sync via this comment: if that backoff changes, update this too — too
|
||||||
|
|||||||
@@ -75,6 +75,65 @@ let initialLoad = false
|
|||||||
// genuine scroll-driven exits mark read after that.
|
// genuine scroll-driven exits mark read after that.
|
||||||
let skipNextObservation = false
|
let skipNextObservation = false
|
||||||
|
|
||||||
|
// Count of fetchData()/sync() calls currently in flight — a refcount rather
|
||||||
|
// than a plain boolean for the same reason as pendingReadCounts below: sync()
|
||||||
|
// awaits its own trailing fetchData(), so one call nests inside the other, and
|
||||||
|
// fetchData() can also be invoked independently (RssFeeds.vue's boot reload)
|
||||||
|
// while a sync() elsewhere is still mid network round-trip. A bare boolean
|
||||||
|
// would have whichever call finishes first clear it out from under the other
|
||||||
|
// still-running one. Both rebuild allItems/feeds from the server, so the
|
||||||
|
// passive/paging mark-as-read paths — handleIntersection's scroll-marking and
|
||||||
|
// markCurrentArticleRead's display-marking — must not fire while any is in
|
||||||
|
// flight. markAllRead is a deliberate, already-confirmed action and is left
|
||||||
|
// ungated.
|
||||||
|
let activeSyncCount = 0
|
||||||
|
function beginSync() {
|
||||||
|
activeSyncCount += 1
|
||||||
|
}
|
||||||
|
// Ids handleIntersection()/markCurrentArticleRead() couldn't mark while
|
||||||
|
// isSyncing() (see below) — flushed once every in-flight fetchData()/sync()
|
||||||
|
// has finished, rather than left unread forever. Flushing by id (not by
|
||||||
|
// re-deriving an index) sidesteps fetchData() having replaced
|
||||||
|
// feeds.value/allItems.value wholesale in the meantime — the id is still
|
||||||
|
// valid, wherever the item now sits.
|
||||||
|
const pendingSuppressedReadIds = new Set()
|
||||||
|
|
||||||
|
function endSync() {
|
||||||
|
activeSyncCount -= 1
|
||||||
|
if (activeSyncCount === 0) {
|
||||||
|
flushPendingSuppressedReads()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
function isSyncing() {
|
||||||
|
return activeSyncCount > 0
|
||||||
|
}
|
||||||
|
|
||||||
|
function flushPendingSuppressedReads() {
|
||||||
|
if (pendingSuppressedReadIds.size === 0) return
|
||||||
|
const ids = [...pendingSuppressedReadIds]
|
||||||
|
pendingSuppressedReadIds.clear()
|
||||||
|
for (const id of ids) {
|
||||||
|
// feeds.value is always a filter()/slice() projection of allItems.value
|
||||||
|
// (see applyFilter), so every item in it is the same object reference as
|
||||||
|
// one already in allItems.value — one lookup is enough.
|
||||||
|
const feed = allItems.value.find(f => f.id === id)
|
||||||
|
if (feed) feed.read = true
|
||||||
|
markRead(id)
|
||||||
|
}
|
||||||
|
// A still-open article view marks in place and relies on dropReadArticles()
|
||||||
|
// running at the next leaveArticleView()/setFeedFilter() to clean up — same
|
||||||
|
// invariant markCurrentArticleRead() already keeps while paging, and
|
||||||
|
// removing one here could shift currentIndex out from under whatever's on
|
||||||
|
// screen. List view has no such upcoming transition to rely on (the user
|
||||||
|
// may just keep scrolling), so sweep these out now: this is exactly what a
|
||||||
|
// scroll-triggered mark would already have done immediately, had a sync not
|
||||||
|
// been in flight.
|
||||||
|
if (viewMode.value !== 'article') {
|
||||||
|
dropReadArticles()
|
||||||
|
}
|
||||||
|
refreshUnreadDisplay()
|
||||||
|
}
|
||||||
|
|
||||||
// Timestamp (performance.now()) of the most recent programmatic scroll / list
|
// Timestamp (performance.now()) of the most recent programmatic scroll / list
|
||||||
// mutation that moves the page without user intent — currently the list-view
|
// mutation that moves the page without user intent — currently the list-view
|
||||||
// read-correction below. AppNav's auto-hide handler resyncs its scroll baseline
|
// read-correction below. AppNav's auto-hide handler resyncs its scroll baseline
|
||||||
@@ -347,8 +406,9 @@ async function setFeedFilter(title) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const fetchData = async () => {
|
const fetchData = async () => {
|
||||||
const user_id = localStorage.getItem("user-id")
|
|
||||||
try {
|
try {
|
||||||
|
beginSync()
|
||||||
|
const user_id = localStorage.getItem("user-id")
|
||||||
// Snapshot ids pending *before* the GET goes out, not just when its
|
// Snapshot ids pending *before* the GET goes out, not just when its
|
||||||
// response comes back. A markRead() PUT that commits while this GET is
|
// response comes back. A markRead() PUT that commits while this GET is
|
||||||
// in flight is invisible to the check below (its id has already been
|
// in flight is invisible to the check below (its id has already been
|
||||||
@@ -378,10 +438,13 @@ const fetchData = async () => {
|
|||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error('Error fetching data:', error)
|
console.error('Error fetching data:', error)
|
||||||
showMessageForXSeconds(error, 5)
|
showMessageForXSeconds(error, 5)
|
||||||
|
} finally {
|
||||||
|
endSync()
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
async function sync(silent = false) {
|
async function sync(silent = false) {
|
||||||
|
beginSync()
|
||||||
try {
|
try {
|
||||||
const response = await axios.post('/api/v1/article/sync', {
|
const response = await axios.post('/api/v1/article/sync', {
|
||||||
user_id: parseInt(localStorage.getItem("user-id"))
|
user_id: parseInt(localStorage.getItem("user-id"))
|
||||||
@@ -390,12 +453,14 @@ async function sync(silent = false) {
|
|||||||
if (response.status == 200 && !silent) {
|
if (response.status == 200 && !silent) {
|
||||||
showMessageForXSeconds('Sync successful.', 5)
|
showMessageForXSeconds('Sync successful.', 5)
|
||||||
}
|
}
|
||||||
fetchData();
|
await fetchData();
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error('Error sync', error)
|
console.error('Error sync', error)
|
||||||
if (!silent) {
|
if (!silent) {
|
||||||
showMessageForXSeconds(error, 5)
|
showMessageForXSeconds(error, 5)
|
||||||
}
|
}
|
||||||
|
} finally {
|
||||||
|
endSync()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -444,6 +509,17 @@ function handleIntersection(entries, topbarHeight = 0) {
|
|||||||
|
|
||||||
if (readFeeds.length === 0) return
|
if (readFeeds.length === 0) return
|
||||||
|
|
||||||
|
if (isSyncing()) {
|
||||||
|
// Don't mutate a list that's mid-reload — queue these ids instead of
|
||||||
|
// dropping them; flushPendingSuppressedReads() marks (and removes) them
|
||||||
|
// once every in-flight fetchData()/sync() has finished. Nothing here
|
||||||
|
// touched the DOM, so there's no observer disconnect/reconnect to do.
|
||||||
|
for (const feed of readFeeds) {
|
||||||
|
pendingSuppressedReadIds.add(feed.id)
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
// Disconnect before the DOM mutation. In card layout the cards are short
|
// Disconnect before the DOM mutation. In card layout the cards are short
|
||||||
// enough that the shift caused by removing one can push the next card above
|
// enough that the shift caused by removing one can push the next card above
|
||||||
// the header, which the observer would immediately treat as another read —
|
// the header, which the observer would immediately treat as another read —
|
||||||
@@ -524,14 +600,20 @@ async function markAllRead() {
|
|||||||
|
|
||||||
function markCurrentArticleRead() {
|
function markCurrentArticleRead() {
|
||||||
const feed = feeds.value[currentIndex.value]
|
const feed = feeds.value[currentIndex.value]
|
||||||
|
if (!feed) return
|
||||||
|
// Don't mark against a list that's mid-reload — see isSyncing/activeSyncCount.
|
||||||
|
// Queue this article's id instead of dropping it: flushPendingSuppressedReads()
|
||||||
|
// marks it once every in-flight fetchData()/sync() has finished.
|
||||||
|
if (isSyncing()) {
|
||||||
|
pendingSuppressedReadIds.add(feed.id)
|
||||||
|
return
|
||||||
|
}
|
||||||
// Marking read here (rather than via removeFeed, as the scroll-based list
|
// Marking read here (rather than via removeFeed, as the scroll-based list
|
||||||
// view does) keeps the array stable so currentIndex stays valid while paging.
|
// view does) keeps the array stable so currentIndex stays valid while paging.
|
||||||
// The local `read` flag lets leaveArticleView() drop these once we're done.
|
// The local `read` flag lets leaveArticleView() drop these once we're done.
|
||||||
if (feed) {
|
|
||||||
feed.read = true
|
feed.read = true
|
||||||
markRead(feed.id)
|
markRead(feed.id)
|
||||||
}
|
}
|
||||||
}
|
|
||||||
|
|
||||||
async function leaveArticleView() {
|
async function leaveArticleView() {
|
||||||
// Articles paged past in article view were marked read but deliberately kept
|
// Articles paged past in article view were marked read but deliberately kept
|
||||||
|
|||||||
Reference in New Issue
Block a user