fix: fix duplicate statuses in threads (#783)

fixes #511

src/routes/_actions/addStatusOrNotification.js

@@ -39,25 +39,39 @@ async function insertUpdatesIntoTimeline (instanceName, timelineName, updates) {
   }
 }
 
+function isValidStatusForThread (thread, timelineName, itemIdsToAdd) {
+  let focusedStatusId = timelineName.split('/')[1] // e.g. "status/123456"
+  let focusedStatusIdx = thread.indexOf(focusedStatusId)
+  return status => {
+    let repliedToStatusIdx = thread.indexOf(status.in_reply_to_id)
+    return (
+      // A reply to an ancestor status is not valid for this thread, but for the focused status
+      // itself or any of its descendents, it is valid.
+      repliedToStatusIdx >= focusedStatusIdx &&
+      // Not a duplicate
+      !thread.includes(status.id) &&
+      // Not already about to be added
+      !itemIdsToAdd.includes(status.id)
+    )
+  }
+}
+
 async function insertUpdatesIntoThreads (instanceName, updates) {
   if (!updates.length) {
     return
   }
 
   let threads = store.getThreads(instanceName)
-
+  let timelineNames = Object.keys(threads)
-  for (let timelineName of Object.keys(threads)) {
+  for (let timelineName of timelineNames) {
     let thread = threads[timelineName]
+
     let itemIdsToAdd = store.getForTimeline(instanceName, timelineName, 'itemIdsToAdd') || []
-    let updatesForThisThread = updates.filter(status => (
+    let validUpdates = updates.filter(isValidStatusForThread(thread, timelineName, itemIdsToAdd))
-      thread.includes(status.in_reply_to_id) &&
+    if (!validUpdates.length) {
-      !thread.includes(status.id) &&
-      !itemIdsToAdd.includes(status.id)
-    ))
-    if (!updatesForThisThread.length) {
       continue
     }
-    let newItemIdsToAdd = uniq(concat(itemIdsToAdd, updatesForThisThread.map(_ => _.id)))
+    let newItemIdsToAdd = uniq(concat(itemIdsToAdd, validUpdates.map(_ => _.id)))
     if (!isEqual(itemIdsToAdd, newItemIdsToAdd)) {
       console.log('adding ', (newItemIdsToAdd.length - itemIdsToAdd.length),
         'items to itemIdsToAdd for thread', timelineName)

tests/spec/122-replies-in-thread.js

@@ -0,0 +1,108 @@
+import { loginAsFoobar } from '../roles'
+import {
+  getNthStatus,
+  getNthStatusContent,
+  getUrl,
+  getNthReplyButton, getNthComposeReplyInput, sleep,
+  getAriaSetSize,
+  getNthComposeReplyButton, goBack, goForward
+} from '../utils'
+import { postAs, postReplyAs } from '../serverActions'
+
+fixture`122-replies-in-threads.js`
+  .page`http://localhost:4002`
+
+const TIMEOUT = 1500
+
+async function goBackAndForward (t) {
+  await goBack()
+  await t.expect(getUrl()).eql('http://localhost:4002/')
+  await goForward()
+  await t.expect(getUrl()).match(/statuses/)
+}
+
+async function verifyAriaSetSize (t, size) {
+  await sleep(TIMEOUT)
+  await t.expect(getAriaSetSize()).eql(size)
+  await goBackAndForward(t)
+  await sleep(TIMEOUT)
+  await t.expect(getAriaSetSize()).eql(size)
+}
+
+// You click on status B, which is a reply to status A.
+// Now status C comes in, which is a response to status A.
+// Status C is inserted automatically, even though it shouldn't be in that thread.
+test('reply to non-focused parent status in a thread', async t => {
+  const { id } = await postAs('admin', 'here is my awesome thread')
+  await postReplyAs('admin', 'and here it is continued', id)
+  await loginAsFoobar(t)
+  await t
+    .hover(getNthStatus(0))
+    .expect(getNthStatusContent(0).innerText).contains('and here it is continued')
+    .click(getNthStatus(0))
+    .expect(getUrl()).match(/statuses/)
+    .click(getNthReplyButton(0))
+    .typeText(getNthComposeReplyInput(0), 'haha I totally agree', { paste: true })
+    .click(getNthComposeReplyButton(0))
+  await verifyAriaSetSize(t, '2')
+})
+
+// In this case it's the same as the previous, except the focused status is status A.
+test('reply to focused status in a thread', async t => {
+  const { id } = await postAs('admin', 'whoa here is my other awesome thread')
+  await postReplyAs('admin', 'whoa and here it is probably continued', id)
+  await loginAsFoobar(t)
+  await t
+    .hover(getNthStatus(0))
+    .expect(getNthStatusContent(0).innerText).contains('whoa and here it is probably continued')
+    .hover(getNthStatusContent(1))
+    .click(getNthStatus(1))
+    .expect(getUrl()).match(/statuses/)
+    .click(getNthReplyButton(0))
+    .typeText(getNthComposeReplyInput(0), 'haha I totally agree', { paste: true })
+    .click(getNthComposeReplyButton(0))
+  await verifyAriaSetSize(t, '3')
+})
+
+// In this case the thread is A-B-C, the focused status is C,
+// and the replied-to status is A.
+test('reply to non-focused grandparent status in a thread', async t => {
+  const { id: id1 } = await postAs('admin', 'cool thread 1/3')
+  const { id: id2 } = await postReplyAs('admin', 'cool thread 2/3', id1)
+  await postReplyAs('admin', 'cool thread 3/3', id2)
+  await loginAsFoobar(t)
+  await t
+    .hover(getNthStatus(0))
+    .expect(getNthStatusContent(0).innerText).contains('cool thread 3/3')
+    .click(getNthStatus(0))
+    .expect(getUrl()).match(/statuses/)
+    .hover(getNthStatus(2))
+    .hover(getNthStatus(1))
+    .hover(getNthStatus(0))
+    .click(getNthReplyButton(0))
+    .typeText(getNthComposeReplyInput(0), 'this is sweet', { paste: true })
+    .click(getNthComposeReplyButton(0))
+  await verifyAriaSetSize(t, '3')
+})
+
+// Thread is A-B-C, focused status is A, replied-to status is C.
+test('reply to non-focused grandchild status in a thread', async t => {
+  const { id: id1 } = await postAs('admin', 'sweet thread 1/3')
+  const { id: id2 } = await postReplyAs('admin', 'sweet thread 2/3', id1)
+  await postReplyAs('admin', 'sweet thread 3/3', id2)
+  await loginAsFoobar(t)
+  await t
+    .hover(getNthStatus(0))
+    .expect(getNthStatusContent(0).innerText).contains('sweet thread 3/3')
+    .hover(getNthStatusContent(1))
+    .hover(getNthStatus(2))
+    .click(getNthStatus(2))
+    .expect(getUrl()).match(/statuses/)
+    .hover(getNthStatus(0))
+    .hover(getNthStatus(1))
+    .hover(getNthStatus(2))
+    .click(getNthReplyButton(2))
+    .typeText(getNthComposeReplyInput(2), 'this is sweet', { paste: true })
+    .click(getNthComposeReplyButton(2))
+  await verifyAriaSetSize(t, '4')
+})

tests/utils.js

@@ -100,6 +100,8 @@ export const getTitleText = exec(() => document.head.querySelector('title').inne
 
 export const goBack = exec(() => window.history.back())
 
+export const goForward = exec(() => window.history.forward())
+
 export const forceOffline = exec(() => window.__forceOnline(false))
 
 export const forceOnline = exec(() => window.__forceOnline(true))
@@ -181,6 +183,10 @@ export function getNthDeleteMediaButton (n) {
   return $(`.compose-media:nth-child(${n}) .compose-media-delete-button`)
 }
 
+export function getAriaSetSize () {
+  return getNthStatus(0).getAttribute('aria-setsize')
+}
+
 export function getNthStatus (n) {
   return $(getNthStatusSelector(n))
 }