stackernews / stacker.news

Internet communities that pay you Bitcoin
https://stacker.news
MIT License
403 stars 105 forks source link

Comments non-deterministically missing after submission #1218

Closed huumn closed 3 weeks ago

huumn commented 3 weeks ago

It happened to @SatsAllDay in response to a post here: https://stacker.news/items/560966/r/k00b?commentId=560967

It happened to me in a response to a comment here: https://stacker.news/items/560297/r/k00b?commentId=561044

ekzyis commented 3 weeks ago

I think this is the cause:

2024-06-03-181343_1053x370_scrot

I encountered this error while working on https://github.com/stackernews/stacker.news/pull/1184 but I thought it was part of my postpaid changes which weren't included in https://github.com/stackernews/stacker.news/pull/1194

ekzyis commented 3 weeks ago

I can reliably reproduce the comment not showing up when I zap the parent first. But the cache warning does not show up. So maybe it's unrelated or another issue. I can't reproduce the cache warning.

huumn commented 3 weeks ago

If I uncomment the optimistic mutation code, it appears to fix it.

ekzyis commented 3 weeks ago

This also seems to fix it (at least for successful zaps):

diff --git a/components/item-act.js b/components/item-act.js
index 29babfd4..f4e360e1 100644
--- a/components/item-act.js
+++ b/components/item-act.js
@@ -277,7 +277,7 @@ export function useZap () {

       let hash, hmac;
       [{ hash, hmac }, cancel] = await payment.request(satsDelta)
-      await zap({ variables: { ...variables, hash, hmac } })
+      await zap({ variables: { ...variables, hash, hmac }, optimisticResponse, update })
     } catch (error) {
       revert?.()

diff --git a/lib/apollo.js b/lib/apollo.js
index d950ed68..17296bc1 100644
--- a/lib/apollo.js
+++ b/lib/apollo.js
@@ -259,7 +259,7 @@ function getClient (uri) {
 export function optimisticUpdate ({ mutation, variables, optimisticResponse, update }) {
   const { cache, queryManager } = getApolloClient()

-  const mutationId = String(queryManager.mutationIdCounter++)
+  const mutationId = String(queryManager.mutationIdCounter)
   queryManager.markMutationOptimistic(optimisticResponse, {
     mutationId,
     document: mutation,

But only removing ++ does not.

I added the ++ in https://github.com/stackernews/stacker.news/commit/a38dc10b7775937277a2977dbcda9845931ac531 for zap undos because iirc, not using ++ meant that the optimistic update for all zaps were undone when one zap was aborted.

Zap undos are lot less important than comments working properly but I'd like to find out if there's a way to get both working. I'd also like to find out what's actually happening instead of speculating why ++ breaks following mutations.

huumn commented 3 weeks ago

All mutations share the queryManager. So not incrementing it means all mutations would share that mutationId.

huumn commented 3 weeks ago

I'm sending you messages in chat. Have you figured that out yet?

ekzyis commented 3 weeks ago

Closing because fixed in #1220