ui: use unique id per connection notification so hearts always fire#364
Merged
Conversation
Discovered via live Chrome DevTools session: only ~50% of successful peer connections produced a heart/explosion animation. The pattern held across a 20-minute session with 6 confirmed connections and 3 "play explosion" log lines. Root cause is the notification id collision: - pushNotification dedups by id: if a queue entry with the same id already exists, it REPLACES that entry in place rather than appending. - useGeoFuture's connection notifications were passing id = geo.workerIdx. - Broflake reuses 8 worker slots. When slot N reconnects while the previous id=N notification is still in the queue (or even being displayed), the dedup silently replaces it. - The Notification effect's `notification.id === notifications[0].id` short-circuit then keeps the Explosion mounted with an unchanged id prop, so the memoed Explosion does not re-render and its [id]-keyed useEffect does not re-fire — no animation, no "play explosion" log. The waiting notification at id=-1 still wants the dedup behavior (don't stack identical "waiting" pushes), so only the per-connection callsite changes. nextNotificationId is a useRef'd monotonic counter, scoped to useGeo so it persists across renders without allocating module state.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes missed heart/explosion animations for repeated peer connections by ensuring per-connection notifications use unique IDs instead of recycled Broflake worker slot IDs.
Changes:
- Adds a monotonic notification ID ref in
useGeo. - Uses that unique ID for successful connection notifications while preserving
id: -1dedup behavior for waiting notifications.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Only ~50% of successful peer connections produced a heart/explosion animation on
unbounded.lantern.io. Discovered via a live Chrome DevTools MCP debugging session: a 20-minute capture with 6 confirmed peer connections generated only 3play explosionlog lines.Root cause is a notification id collision. The fix gives each per-connection notification a unique id.
How the bug manifests
sequenceDiagram autonumber participant W as Broflake WASM<br/>wasmInterface.ts:225 participant E as connectionsEmitter participant G as useGeoFuture<br/>useGeoFuture.ts:232 participant Q as notificationQueue<br/>notification/index.tsx:21 participant N as Notification effect<br/>notification/index.tsx:41 participant X as Explosion useEffect<br/>explosion.tsx:9 Note over W,X: Slot N connects, gets shown, then slot N is reused while the queue still holds id=N W->>E: handleConnection({wi:3, state:1}) E->>G: updateArcs sees state=1 for wi=3 G->>Q: pushNotification({id:3, heart:true}) Q->>N: queue=[A:3] N->>X: render Explosion(id=3) X-->>X: useEffect fires — "play explosion" ✓ Note over W: ~1s later, slot 3 disconnects then reconnects W->>E: handleConnection({wi:3, state:-1}) W->>E: handleConnection({wi:3, state:1}) E->>G: updateArcs sees state=1 for wi=3 again G->>Q: notification/index.tsx:22<br/>pushNotification({id:3, heart:true}) ⚠️ rect rgba(255, 200, 200, 0.3) Note over Q: notification/index.tsx:23-25<br/>findIndex(n.id === 3) → REPLACES existing<br/>queue entry instead of appending 🐛 Note over N: notification/index.tsx:52<br/>notification.id === notifications[0].id<br/>(both 3) → early return 🐛 end N--xX: Explosion stays mounted with id=3 unchanged X--xX: memo prevents re-render — useEffect never re-fires Note over X: no "play explosion", no animationWhy workerIdx looked like a reasonable id
It looks unique per connection at a glance — but Broflake reuses 8 worker slots (
Starting WorkerFSM... [8 times]in the WASM init logs). So the sameworkerIdxvalue cycles through many connections over a session. ThepushNotificationdedup (which is needed forid: -1"waiting" notifications) silently kills connection notifications.Fix
useGeoFuture.ts:The
id: -1waiting notification atuseGeoFuture.ts:264is left alone — it wants dedup so multiple "waiting" pushes don't stack. Only the per-connection callsite needed a unique id.Test plan
yarn build:websucceeds,main.jsproduced withpublicPath=https://embed.lantern.io/baked in.unbounded.lantern.ioin Chrome with the new bundle, toggle sharing, wait for ≥3 successful peer connections. Each connection should produce a"play explosion"console log and a visible heart animation overlaying the globe.