Spoke: Bug: "Skip Reply" button missing

[jeffm2001]

Describe the bug
On the "Send Replies" screen, texters do not see the Skip Reply button, at least initially. This means they end up with a backlog of "needs response" conversations they need to page through in order to get to the newest replies.

After some time (a few hours) the Skip Reply button does usually appear. I have also confirmed that restarting the server immediately resolves the issue.

This _feels_ like a caching issue to me, where the frontend is not seeing the updated message status, but I'm not well versed enough in the Spoke architecture yet to definitively figure out what's going on.

To Reproduce
Steps to reproduce the behavior:

1. Send some first texts
2. Wait for responses
3. Click Send Replies
4. See that the Skip Reply button is missing

Screenshots

Platform (please complete the following information):

• OS: Mac and iOS
• Browser: Chrome and Safari
• Desktop or Mobile? Both
• Version 3.1

Additional context
I've replicated this issue both on our production instance on Heroku and a test instance running on my local machine.

[joemcl]

@jeffm2001 yeah I'm seeing this now as well. But restarting dynos did not fix the issue. I'm running upstream main from like Sept 12th, on Heroku.

[joemcl]

@lperson @schuyler1d @ibrand thoughts on this??

[lperson]

@joemcl is this happening on wfp spoke in production?

[joemcl]

@lperson happening on another Spoke instance in production, yes

[joemcl]

so restarted the dyno and Skip Reply is back

[lperson]

@jeffm2001 what version of the code are you running?

[lperson]

And, the good news ... I just reproduced this in main!

[joshco]

here too

[joshco]

looks like renderNeedsResponseToggleButton handles status [ 'closed', 'needsResponse' ] with explicit if, else if, but no default else case. So if it's another state, no button at all is rendered, it's null..

https://github.com/MoveOnOrg/Spoke/blob/ce8dfd15ac8bc231f9fb4a4aaedad5b4a96b405b/src/containers/AssignmentTexterContact.jsx#L651-L672

messagestatus can also be [ 'messaged', 'convo' ] at least from what I see.

[lperson]

@joshco looking into why the server is incorrectly returning status as messaged when in the database it's needsResponse

[joshco]

@lperson is that a cached query?
either way, there still needs to be a default case. Seems like if the UI is showing the conversation anyway, and it's not "closed", then the skip button might as well be there

[lperson]

@joshco I don't believe spoke wants testers skipping initial messages

[joshco]

@joshco I don't believe spoke wants testers skipping initial messages
Lol yeah, that's probably considered cheating. Or shenanigans as Organizer calls it.

[joshco]

@lperson i see the same thing in the database. "messaged" wierd

[lperson]

@joshco The bug is about contacts that received a message and sent a reply. If you're seeing "messaged" in campaign_contact the contact didn't send a reply. Are you sure you're looking at a contact that sent a reply?

[joshco]

@lperson I noticed this line: https://github.com/MoveOnOrg/Spoke/blob/ce8dfd15ac8bc231f9fb4a4aaedad5b4a96b405b/src/server/api/schema.js#L1158

I'm looking at my postgres table and seeing "needsResponse", but in my chrome developer tool I'm seeing "messaged"

POSTGRES

CHROME

[lperson]

~Yes @joshco. That's causing the bug.~

Fake news.

[lperson]

@joshco if you look at the updated_at field in the database vs. the updated_at field in the browser object, if the bug is reproduced they are different. The database will be more recently updated -- it will have a more recent timestamp and the status will be needsResponse not messaged. So if you start the server, send the first message and then the contact sends a reply, you'll see updated_at in the database more recent than updated_at in the data sent to the browser.

I believe the reason for this is that the data is being loaded by the DataLoader class. Look at where the data is loaded. The loader in that line is created here through a call to a function here.

[lperson]

@joshco Continuing that thought, DataLoader has a memoization cache. I believe what's happening is that after the messageStatus is updated in the database, when the contact is retrieved after clicking through [Send Replies] the data read is intercepted by DataLoader, serviced from the memoization cache, and it never goes to the database to get the latest.

[joshco]

@lperson maybe whats happening is that the line I mentioned is ..... yeah that's what i was looking at
the line i mentioned is saving the contact with a different state, but the client doesnt update.
What about changing the line 661 of AssignmentTexterContact.jsx
from
} else if (messageStatus === "needsResponse") {
to
} else if (messageStatus === "needsResponse" || messageStatus === 'messaged') {

[lperson]

@joshco that line is in code related to sending the message. It is correctly updating messageStatus after sending a message. I am certain that line is not the culprit.

[joshco]

@lperson that's not what I mean. I'm talking about on the client side, if it still sees the status as 'messaged' treat it the same as 'needsResponse' that shows the skip reply button for those cases, but not the initial message state

[joshco]

@lperson I'm talking about line 661 below. Saw your PR, that's smart too.

https://github.com/MoveOnOrg/Spoke/blob/ce8dfd15ac8bc231f9fb4a4aaedad5b4a96b405b/src/containers/AssignmentTexterContact.jsx#L651-L668

[lperson]

@joshco Ok, I see what you're saying. I think it makes more sense to send the accurate state to the client though.

[lperson]

@joshco continuing that thought, modifying behavior in the client to work around not having up-to-date state doesn't feel like an actual fix; also, we'd want state to be returned accurately because we don't want a situation to arise in the future where we again have to ask "why is this not being rendered properly in the client?"

[joshco]

@lperson I agree. It's not an either/or. That if statement that only covers 2 cases is a race condition waiting to happen. :) It's probably overkill given your fix.

[lperson]

@joshco yeah that's a really good point. I think the assumption there is that contacts in other states would never be displayed in the texter's view of the world. There's no use case for it; texters can't load contacts unless they need an initial message or need a reply.

[joshco]

@lperson yeah, I agree, but I'll bet you $10 that it wont be all that long before we are asking ourselves "why is this not being rendered properly in the client?"
People are often on slow interconnections, overcrowded wifi access points in crowded events. networks are unreliable etc etc.
Postel's law: https://en.wikipedia.org/wiki/Robustness_principle

[joshco]

@lperson anyway, good fix.

[joemcl]

thanks so much @lperson and @joshco , cc: @jeffm2001

[lperson]

@joemcl i need to iterate on this. I slept on it and there’s a better fix. It’s not ready to merge.

[lperson]

@jeffm2001 is your Spoke deployed on Lambda?

[jeffm2001]

@lperson no, Heroku

[lperson]

Some clarifications about the analysis for this. DataLoader does not have an MRU cache as far as I can tell. This bug happens because we're not updating the cache after processing incoming messages. (As I described above, when we process incoming messages we change message_status in campaign_contact. That's done directly in the database; the cache is not updated. Even if we did update the cache, the update could possibly happen in a separate process -- incomingMessageHandler --, so the cache would be inconsistent between processes.)

See PR #1255 for a discussion about tradeoffs involved in the fix.

FYI @joshco and @jeffm2001