search

planetary-social / nos

nos.social social media for all of us, using nostr
https://nos.social
Mozilla Public License 2.0
123 stars 14 forks source link

Some notes show spinners and never load #703

Closed mplorentz closed 5 months ago

mplorentz commented 12 months ago

When browsing any feed sometimes a repost, linked note, or root note will fail to load and just show a spinner. We should make sure these always load if they are on a relay the current user uses.

I think this is caused by duplicate objects being created in the database which I wrote more about in https://github.com/planetary-social/nos/pull/696.

setch-l commented 11 months ago

This is still not fixed for posts by Charlie Stross on Build 112.

06799223-73DE-4924-B37A-7DE9F1EC1BAE.zip

Image

mplorentz commented 10 months ago

This is happening a lot on build 168, and we are seeing intermittent EventObservationTests failures on CI which also indicates that this is still an issue. Reopening.

mplorentz commented 10 months ago

I've disabled the EventObservationTests for now. make sure to re-enable them before closing this ticket.

mplorentz commented 10 months ago

This is still happening. I did some more investigation but I'm pretty out of ideas on how to fix this. I still feel like the root issue is that duplicate Events and Authors are getting created in separate NSManagedObjectContexts (like the parseContext and viewContext) and then when they are merged the resulting data in sqlite is correct, but the SwiftUI views don't always get notified of the changes and update accordingly. So when we create a stubbed Event for a reposted note we haven't downloaded yet sometimes even after we have downloaded it SwiftUI doesn't display it.

One potential temporary hack would be to just have NoteCard refetch the Event from the database every few seconds. But this might cause performance problems and doesn't solve the real issue.

mplorentz commented 8 months ago

Maybe if we create Events and Authors using SwiftData we won't run into the deadlock? Something we could think about alongside #971.

setch-l commented 6 months ago

@mplorentz - I think this is primarily two scenarios:

For this ticket - let's focus on these two scenarios and if we have some remaining - we can create a new ticket for the next sprint.

rabble commented 6 months ago

Should we look at the solution of republishing notes to our relays when we republish a note. This means folks who find the repost event would be able to get the reposted event from that same relay.

mplorentz commented 6 months ago

I've completed my investigation and I found three problems in the code and identified some potential solutions. I'll start implementing the solutions now I'm fairly confident these will fix the majority of the spinners for content that is present on the user's relays.

For content not on the user's relays we are really straying into the territory of #647. I don't want to try to fix those cases as part of this ticket, because it's a lot more work and really we just need to prioritize #647 instead of applying more band-aids to our overly naive RelayService. But I will go ahead and write up some tickets for some improvements like the one from @rabble above that will help in this situation.

The three problems I referenced above are:

  1. The database cleanup script is deleting EventReferences and replacing them with new ones, breaking SwiftUI observation. Solution: refactor EventReferences out of model layer.
  2. Using an NSManagedObject as a SwiftUI .navigationDestination breaks object observation. Solution: use IDs as the navigation destinations and set up container views that observe the object with the give ID.
  3. Reposts with empty content field show a loading state without a spinner. Solution: TBD.
setch-l commented 6 months ago

@mplorentz - It's not clear to me why issue 3 is happening. Is it because we are not republishing the repost and can't find it? Or is that a separate issue?

mplorentz commented 6 months ago

@setch-l that's a separate issue. We actually embed the reposted note inside the "repost" event when we publish them. But not every app does this, and it seems like we aren't properly handling events that don't.

mplorentz commented 5 months ago

It turns out that problem 3 doesn't have anything to do with the content field, and it doesn't impact our loading of notes. It's some kind of SwiftUI bug. As such it doesn't feel like it fits within this ticket and it feels much less important. I opened #1257 to track that issue specifically. CC @setch-l so you can set priority.

I also found another unrelated bug while working on this, which I wrote up in #1258.