Skip to content

Fix crash when an open :show page gets a PubSub broadcast for other items#6197

Merged
SteffenDE merged 1 commit intophoenixframework:mainfrom
nshafer:ns_fix_crash_for_other_items
Apr 15, 2025
Merged

Fix crash when an open :show page gets a PubSub broadcast for other items#6197
SteffenDE merged 1 commit intophoenixframework:mainfrom
nshafer:ns_fix_crash_for_other_items

Conversation

@nshafer
Copy link
Copy Markdown
Contributor

@nshafer nshafer commented Apr 15, 2025

In the code generated with mix phx.gen.live, in the show.ex file, there are 2 handle_info/2 callbacks that are tightly bound to only match when the messages broadcast from the context are for the item they are currently showing. If the user has another tab open on a show page, and does anything from another tab, such as adding, deleting or updating a different item, the show LV will crash with a "no function claus matching" exception.

This simply adds a fallback handle_info/2 handler that tightly matches all other expected messages due to the Context.subscribe_items/1 call in the generated mount/3, so that LV won't crash when it gets an unexpected message. However, if it gets a different message that is from something else that the user adds, then it will still crash so they know they need to handle that message shape.

@SteffenDE
Copy link
Copy Markdown
Contributor

Thank you! Sure feels like we should have caught that in review 😅

@SteffenDE
Copy link
Copy Markdown
Contributor

@josevalim should we have a separate topic that only broadcasts messages for a specific ID?

@josevalim
Copy link
Copy Markdown
Member

The issue with doing it per ID is that we deliver double messages? Another option is to deal with updates only on the index page for the scaffold.

@SteffenDE
Copy link
Copy Markdown
Contributor

How expensive if sending a PubSub message to a topic where nobody is subscribed? It's only the messages to the other nodes, right? Having update/delete broadcast to two topics is a pattern that I frequently used already. But the catch all might be just fine here? I think it depends on which way we want to push people towards.

I think if we do PubSub, all pages should update, so I wouldn't exclude the show page.

@josevalim
Copy link
Copy Markdown
Member

Yes, the issue is when you have multiple nodes, two messages across the nodes. The catch-all is fine here. :)

@SteffenDE SteffenDE merged commit 157dd54 into phoenixframework:main Apr 15, 2025
5 checks passed
@SteffenDE
Copy link
Copy Markdown
Contributor

Let's go with this one then :)

Thank you @nshafer!

@nshafer
Copy link
Copy Markdown
Contributor Author

nshafer commented Apr 15, 2025

No problem, thank you both. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants