Merge pull request 'Fix unread counter stuck after reading all articles' (#2) from hotfix/unread-counter-stuck into master

Reviewed-on: #2

vue/src/composables/__tests__/useFeeds.spec.js

@@ -1,5 +1,4 @@
-import { describe, it, expect, vi, beforeEach } from 'vitest'
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
-import { flushPromises } from '@vue/test-utils'
 import axios from 'axios'
 import { useFeeds } from '../useFeeds'
 
@@ -13,7 +12,7 @@ class FakeIntersectionObserver {
 vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver)
 
 describe('useFeeds', () => {
-  const { feeds, allItems, feedFilter, feedTitles, feedUnreadCounts, unreadCount, displayedFeedUnreadCounts, refreshUnreadDisplay, setFeedFilter, showMessage, message, showModal, fetchData, sync, getReadable, setInitialLoad, handleIntersection, markAllRead, setupIntersectionObserver } = useFeeds()
+  const { feeds, allItems, feedFilter, feedTitles, feedUnreadCounts, unreadCount, displayedFeedUnreadCounts, refreshUnreadDisplay, setFeedFilter, showMessage, message, showModal, fetchData, sync, getReadable, setInitialLoad, handleIntersection, markAllRead, markRead, setupIntersectionObserver } = useFeeds()
 
   beforeEach(() => {
     localStorage.setItem('user-token', 'test-token')
@@ -178,6 +177,193 @@ describe('useFeeds', () => {
     setInitialLoad(false)
   })
 
+  describe('markRead resilience', () => {
+    // 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
+    // small a value just makes the affected test's `await` hang until it
+    // times out, rather than pass incorrectly.
+    const RETRY_BACKOFF_SUM_MS = 300 + 1000
+
+    // A failed assertion before a test's own vi.useRealTimers() call would
+    // otherwise leak fake timers into every later test in this file.
+    afterEach(() => {
+      vi.useRealTimers()
+    })
+
+    it('retries a failed mark-read PUT before giving up, with a request timeout set', async () => {
+      vi.useFakeTimers()
+      axios.put
+        .mockRejectedValueOnce(new Error('network'))
+        .mockRejectedValueOnce(new Error('network'))
+        .mockResolvedValueOnce({ status: 200 })
+
+      const done = markRead(601)
+      await vi.runAllTimersAsync()
+
+      expect(await done).toBe(true)
+      expect(axios.put).toHaveBeenCalledTimes(3)
+      expect(axios.put).toHaveBeenCalledWith(
+        '/api/v1/article/read/601',
+        null,
+        expect.objectContaining({ timeout: expect.any(Number) }),
+      )
+      expect(showMessage.value).toBe(false)
+    })
+
+    it('reports failure once retries are exhausted, and stops withholding the item from a refetch', async () => {
+      vi.useFakeTimers()
+      axios.put.mockRejectedValue(new Error('network'))
+
+      const done = markRead(602)
+      // Advance past just the two retry backoffs — not runAllTimersAsync(),
+      // which would also fire showMessageForXSeconds's own 5s auto-hide timer
+      // and reset showMessage before the assertion below.
+      await vi.advanceTimersByTimeAsync(RETRY_BACKOFF_SUM_MS)
+
+      expect(await done).toBe(false)
+      expect(axios.put).toHaveBeenCalledTimes(3)
+      expect(showMessage.value).toBe(true)
+      expect(message.value).toMatch(/could not mark/i)
+
+      // The mark never committed, so the item genuinely is still unread
+      // server-side — a refetch must be free to show it again, not hide it
+      // forever as "pending".
+      axios.get.mockResolvedValueOnce({
+        data: {
+          feeds: [{
+            title: 'Feed A',
+            items: [{ id: 602, title: 'A1', content: '', url: 'https://example.test/a1', timestamp: '2026-01-01 10:00:00' }],
+          }],
+        },
+      })
+      await fetchData()
+      expect(feeds.value.map(f => f.id)).toEqual([602])
+    })
+
+    it('does not retry a non-retryable 4xx mark-read failure', async () => {
+      const error = new Error('Not Found')
+      error.response = { status: 404 }
+      axios.put.mockRejectedValueOnce(error)
+
+      const result = await markRead(605)
+
+      expect(result).toBe(false)
+      expect(axios.put).toHaveBeenCalledTimes(1)
+      expect(showMessage.value).toBe(true)
+    })
+
+    it('keeps a still-unconfirmed read article out of a concurrent background refetch', async () => {
+      let resolvePut
+      axios.put.mockReturnValueOnce(new Promise(resolve => { resolvePut = resolve }))
+
+      const done = markRead(603) // simulates a page-turn's fire-and-forget mark-read, still in flight
+
+      // A background sync's trailing fetchData() resolves while that PUT is
+      // still pending — the server hasn't committed the read yet, so it's
+      // still in the response.
+      axios.get.mockResolvedValueOnce({
+        data: {
+          feeds: [{
+            title: 'Feed A',
+            items: [{ id: 603, title: 'A1', content: '', url: 'https://example.test/a1', timestamp: '2026-01-01 10:00:00' }],
+          }],
+        },
+      })
+      await fetchData()
+
+      expect(feeds.value).toEqual([])
+
+      resolvePut({ status: 200 })
+      await done
+    })
+
+    it('excludes an item whose mark-read PUT settles between a refetch\'s request and its response', async () => {
+      // The PUT commits (and clears the pending id) while the GET below is
+      // still in flight — the GET's response nonetheless reflects the DB as
+      // of before that commit, still reporting the item unread. A snapshot
+      // taken only once the GET resolves would miss this; one taken before
+      // it's dispatched (unioned with the live state) catches it too.
+      axios.put.mockResolvedValueOnce({ status: 200 })
+      const markDone = markRead(604)
+
+      let resolveGet
+      axios.get.mockReturnValueOnce(new Promise(resolve => { resolveGet = resolve }))
+      const fetchPromise = fetchData()
+
+      await markDone // the PUT (and pending-id cleanup) settles first...
+
+      resolveGet({ // ...then the already-in-flight GET resolves, still unread.
+        data: {
+          feeds: [{
+            title: 'Feed A',
+            items: [{ id: 604, title: 'A1', content: '', url: 'https://example.test/a1', timestamp: '2026-01-01 10:00:00' }],
+          }],
+        },
+      })
+      await fetchPromise
+
+      expect(feeds.value).toEqual([])
+    })
+
+    it('keeps an item withheld until every overlapping markRead() call for it settles', async () => {
+      // nextArticle()/prevArticle() can both re-mark the same article (page
+      // forward, back, forward again) — two overlapping calls for one id.
+      let resolveFirst
+      let resolveSecond
+      axios.put
+        .mockReturnValueOnce(new Promise(resolve => { resolveFirst = resolve }))
+        .mockReturnValueOnce(new Promise(resolve => { resolveSecond = resolve }))
+
+      const first = markRead(606)
+      const second = markRead(606)
+
+      resolveSecond({ status: 200 }) // the second (later) call settles first
+      await second
+
+      // The first call is still in flight — a refetch must still withhold it.
+      axios.get.mockResolvedValueOnce({
+        data: {
+          feeds: [{
+            title: 'Feed A',
+            items: [{ id: 606, title: 'A1', content: '', url: 'https://example.test/a1', timestamp: '2026-01-01 10:00:00' }],
+          }],
+        },
+      })
+      await fetchData()
+      expect(feeds.value).toEqual([])
+
+      resolveFirst({ status: 200 })
+      await first
+    })
+  })
+
+  describe('markAllRead resilience', () => {
+    const RETRY_BACKOFF_SUM_MS = 300 + 1000
+
+    afterEach(() => {
+      vi.useRealTimers()
+    })
+
+    it('reports a partial failure instead of claiming full success', async () => {
+      feeds.value = [
+        { id: 701, title: 'First' },
+        { id: 702, title: 'Second' },
+      ]
+      vi.spyOn(window, 'confirm').mockReturnValue(true)
+      axios.put
+        .mockResolvedValueOnce({ status: 200 }) // 701 succeeds
+        .mockRejectedValue(new Error('network')) // 702 fails every attempt
+
+      vi.useFakeTimers()
+      const done = markAllRead()
+      await vi.advanceTimersByTimeAsync(RETRY_BACKOFF_SUM_MS)
+      await done
+
+      expect(message.value).toContain('1 of 2')
+      expect(message.value).not.toBe('All articles marked as read.')
+    })
+  })
+
   describe('feed filter', () => {
     const twoFeedsResponse = {
       data: {

vue/src/composables/useFeeds.js

@@ -228,12 +228,74 @@ async function getReadable(feed, index) {
   }
 }
 
+// Ids with a mark-read PUT in flight (or being retried) — see markRead() and
+// fetchData() below. A refcount rather than a plain Set: nextArticle()/
+// prevArticle() can both re-mark the same article (page forward, back,
+// forward again), so two overlapping markRead() calls for one id must both
+// finish before fetchData() is allowed to trust the server's word on it —
+// otherwise the first call to settle would clear the id out from under the
+// second, still-in-flight one.
+const pendingReadCounts = new Map()
+
+function addPendingRead(id) {
+  pendingReadCounts.set(id, (pendingReadCounts.get(id) ?? 0) + 1)
+}
+
+function removePendingRead(id) {
+  const count = pendingReadCounts.get(id) ?? 0
+  if (count <= 1) {
+    pendingReadCounts.delete(id)
+  } else {
+    pendingReadCounts.set(id, count - 1)
+  }
+}
+
+function delay(ms) {
+  return new Promise(resolve => setTimeout(resolve, ms))
+}
+
+// Short backoff between retries of a failed mark-read PUT — each entry is the
+// wait before that retry attempt.
+const MARK_READ_RETRY_DELAYS_MS = [300, 1000]
+// Bounds how long a hung request (dead connection, captive portal — axios has
+// no default timeout) can keep an id "pending": without this, a request that
+// never settles would never leave pendingReadCounts, silently withholding
+// that article from every fetchData() for the rest of the session.
+const MARK_READ_TIMEOUT_MS = 10000
+
+// 4xx responses (expired token, item not found/not owned — see
+// src/reader/mark_read.rs) mean the request itself is wrong and won't
+// succeed on retry; only a missing response (network error, timeout) or a
+// server-side/rate-limit status is worth retrying.
+function isRetryableMarkReadError(error) {
+  const status = error.response?.status
+  return status === undefined || status >= 500 || status === 429
+}
+
+// Resolves to true once the article is confirmed read server-side, or false
+// once retries (if any) are exhausted / the error isn't retryable.
 async function markRead(id) {
+  addPendingRead(id)
   try {
-    const response = await axios.put("/api/v1/article/read/" + id, null, authHeaders())
+    for (let attempt = 0; ; attempt++) {
-    console.log(response.status)
+      try {
+        await axios.put("/api/v1/article/read/" + id, null, { ...authHeaders(), timeout: MARK_READ_TIMEOUT_MS })
+        return true
       } catch (error) {
-    console.log(error)
+        console.error('Error marking article read:', error)
+        if (attempt >= MARK_READ_RETRY_DELAYS_MS.length || !isRetryableMarkReadError(error)) {
+          // Out of retries, or a retry can't help — this mark never committed
+          // server-side, so the item genuinely is still unread there. Surface
+          // it rather than letting it silently vanish from the local list now
+          // only to mysteriously reappear as unread later.
+          showMessageForXSeconds('Could not mark an article as read. It may reappear as unread.', 5)
+          return false
+        }
+        await delay(MARK_READ_RETRY_DELAYS_MS[attempt])
+      }
+    }
+  } finally {
+    removePendingRead(id)
   }
 }
 
@@ -265,15 +327,28 @@ async function setFeedFilter(title) {
 const fetchData = async () => {
   const user_id = localStorage.getItem("user-id")
   try {
+    // Snapshot ids pending *before* the GET goes out, not just when its
+    // 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
+    // cleared by the time the response arrives) even though the server read
+    // the DB before that commit and so still reported the item unread — this
+    // snapshot catches that case too.
+    const pendingBeforeFetch = new Set(pendingReadCounts.keys())
     const response = await axios.get("/api/v1/article/get/" + user_id, authHeaders());
     const items = [];
     response.data.feeds.forEach(feed => {
       feed.items.forEach(item => items.push({ ...item, feedTitle: feed.title }));
     });
+    // An item pending at either end of this request may not have committed
+    // server-side yet, so the server can still report it unread here. Drop it
+    // from this snapshot too — otherwise a concurrent background sync's
+    // refetch (see sync()) would resurrect it as unread out from under the
+    // user while markRead() is still confirming or retrying it.
+    const freshItems = items.filter(item => !pendingBeforeFetch.has(item.id) && !pendingReadCounts.has(item.id));
     // timestamps are zero-padded "YYYY-MM-DD HH:MM:SS" strings, so a plain
     // lexicographic comparison sorts them chronologically.
-    items.sort((a, b) => b.timestamp.localeCompare(a.timestamp));
+    freshItems.sort((a, b) => b.timestamp.localeCompare(a.timestamp));
-    allItems.value = items;
+    allItems.value = freshItems;
     applyFilter();
     refreshUnreadDisplay();
     await nextTick();
@@ -411,9 +486,18 @@ async function markAllRead() {
   allItems.value = allItems.value.filter(feed => !readIds.has(feed.id))
   currentIndex.value = 0
   refreshUnreadDisplay()
-  // markRead swallows its own errors, so Promise.all can't reject here.
+  // markRead() resolves to true/false rather than rejecting, so Promise.all
-  await Promise.all(ids.map(id => markRead(id)))
+  // can't reject here — but a false means that article is still unread
+  // server-side, which the message below must not contradict.
+  const results = await Promise.all(ids.map(id => markRead(id)))
+  const failedCount = results.filter(ok => !ok).length
+  if (failedCount === 0) {
     showMessageForXSeconds('All articles marked as read.', 5)
+  } else {
+    // markRead() already surfaced each individual failure — this summarizes
+    // the batch outcome instead of claiming full success over it.
+    showMessageForXSeconds(`Marked ${ids.length - failedCount} of ${ids.length} as read; ${failedCount} failed and may reappear.`, 5)
+  }
 }
 
 function markCurrentArticleRead() {