fix: stream chat loading improvements#54
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes aimed at improving stream/chat loading behavior and stabilizing player/widget rendering, including updates to the HLS player dependency and several React component refactors.
Changes:
- Bump
hls-video-element(and related deps) and adjust the live video player implementation. - Add additional request filters in stream state to load chat/zaps for more link types.
- Memoize several widget/row components and add some UI/accessibility tweaks in Widgets page.
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile entries for hls-video-element, custom-media-element, media-tracks. |
| package.json | Bumps hls-video-element to ^1.5.11. |
| tsconfig.json | Changes TS target/class-fields emit settings; removes explicit moduleResolution: bundler. |
| src/pages/widgets/zaps.tsx | Avoids mutating input array in queue logic; memoizes widget components. |
| src/pages/widgets/views.tsx | Memoizes Views widget and reformats imports. |
| src/pages/widgets/top-zappers.tsx | Memoizes TopZappers widget and reformats imports. |
| src/pages/widgets/music.tsx | Memoizes MusicWidget and reformats imports. |
| src/pages/widgets.tsx | Memoizes WidgetsPage, adjusts TTS toggle UI, and memoizes helper component/params. |
| src/pages/stream-page.tsx | Forces LiveVideoPlayer remount on link change; adds debug logging. |
| src/element/zapper-row.tsx | Memoizes ZapperRow and reformats. |
| src/element/top-zappers.tsx | Memoizes TopZappers and reformats. |
| src/element/stream/stream-state.tsx | Adds non-replyToLink filters for additional link types. |
| src/element/stream/n94-player.tsx | Improves ref typing and ensures mpegts player is destroyed/cleaned up. |
| src/element/stream/moq-player.tsx | Trivial formatting change. |
| src/element/stream/live-video-player.tsx | Significant refactor for HLS + added diagnostics and cast availability handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let castSubscription: (() => void) | undefined | ||
| if (playerRef.current && 'remote' in playerRef.current) { | ||
| // @ts-expect-error | ||
| castSubscription = playerRef.current.remote.watchAvailability(a => { | ||
| console.debug('Cast support: ', a) | ||
| setCanCast(a) | ||
| }) | ||
| } | ||
|
|
||
| // Cleanup when component unmounts | ||
| return () => { | ||
| if (castSubscription) { | ||
| castSubscription() |
There was a problem hiding this comment.
The cleanup logic for remote.watchAvailability is incorrect: watchAvailability(...) does not return an unsubscribe function, so storing it as () => void and calling it on cleanup will throw at runtime. Use the API’s actual cancellation mechanism (e.g., store the returned watch id/promise result and call cancelWatchAvailability(id)), and avoid @ts-expect-error here so TypeScript can enforce the correct contract.
| let castSubscription: (() => void) | undefined | |
| if (playerRef.current && 'remote' in playerRef.current) { | |
| // @ts-expect-error | |
| castSubscription = playerRef.current.remote.watchAvailability(a => { | |
| console.debug('Cast support: ', a) | |
| setCanCast(a) | |
| }) | |
| } | |
| // Cleanup when component unmounts | |
| return () => { | |
| if (castSubscription) { | |
| castSubscription() | |
| if (!playerRef.current || !('remote' in playerRef.current)) { | |
| return | |
| } | |
| const remote = (playerRef.current as any).remote as RemotePlayback | undefined | |
| if (!remote || typeof remote.watchAvailability !== 'function') { | |
| return | |
| } | |
| const handleAvailability = (available: boolean) => { | |
| console.debug('Cast support: ', available) | |
| setCanCast(available) | |
| } | |
| remote | |
| .watchAvailability(handleAvailability) | |
| .catch(error => { | |
| console.debug('Error watching cast availability:', error) | |
| }) | |
| // Cleanup when component unmounts | |
| return () => { | |
| if (typeof remote.cancelWatchAvailability === 'function') { | |
| try { | |
| remote.cancelWatchAvailability(handleAvailability) | |
| } catch (error) { | |
| console.debug('Error cancelling cast availability watch:', error) | |
| } |
| </div> | ||
| {innerPlayer()} | ||
| <MediaRenditionMenu hidden anchor="auto" /> | ||
| <div style={{ border: '2px solid blue', minHeight: '200px' }}>{innerPlayer()}</div> |
There was a problem hiding this comment.
MediaController slotting only works with direct children. Wrapping innerPlayer() inside an extra <div> means the media element with slot="media" is no longer a direct child of MediaController, so controls may not attach to the media element and playback can break. Render the slotted media element directly under MediaController (remove the wrapper) and move any diagnostics outside the controller or into slot="top-chrome"/slot="centered-chrome" if needed.
| <div style={{ border: '2px solid blue', minHeight: '200px' }}>{innerPlayer()}</div> | |
| {innerPlayer()} |
| {status === StreamState.Live && <MediaRenditionMenuButton />} | ||
| <MediaPipButton /> | ||
| {/* {status === StreamState.Live && <MediaRenditionMenuButton />} */} | ||
| <MediaPlayButton /> |
There was a problem hiding this comment.
There are now two <MediaPlayButton /> controls in the same MediaControlBar (one at the start and another near the end). This will render duplicate play/pause buttons and is likely accidental; keep a single play button and restore the intended controls (PiP/rendition/etc.) if still required.
| <MediaPlayButton /> |
| interface VideoPlayerProps { | ||
| title?: string | ||
| stream?: string | ||
| status?: StreamState | ||
| poster?: string | ||
| link?: NostrLink | ||
| [key: string]: any | ||
| } |
There was a problem hiding this comment.
VideoPlayerProps makes link optional and adds [key: string]: any, which drops type safety and allows passing undefined into Nip94Player even though it requires a NostrLink. Keep link required (or guard the render) and avoid the any index signature by extending the appropriate element prop types / defining a proper type for the custom element.
| console.debug('StreamPageContent: link:', link) | ||
| console.debug('StreamPageContent: info:', info) | ||
| console.debug('StreamPageContent: event:', event) | ||
| console.debug('StreamPageContent: goal:', goal) |
There was a problem hiding this comment.
These console.debug statements will run on every render and can leak potentially sensitive stream metadata into end-user consoles. Please remove them or guard them behind a debug/dev flag before merging.
| console.debug('StreamPageContent: link:', link) | |
| console.debug('StreamPageContent: info:', info) | |
| console.debug('StreamPageContent: event:', event) | |
| console.debug('StreamPageContent: goal:', goal) | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.debug('StreamPageContent: link:', link) | |
| console.debug('StreamPageContent: info:', info) | |
| console.debug('StreamPageContent: event:', event) | |
| console.debug('StreamPageContent: goal:', goal) | |
| } |
| <input | ||
| type="checkbox" | ||
| checked={textToSpeech} | ||
| onChange={ev => { | ||
| setTextToSpeech(ev.target.checked); | ||
| setTextToSpeech(ev.target.checked) | ||
| }} | ||
| className="hidden" | ||
| /> |
There was a problem hiding this comment.
The checkbox is now className="hidden" and the click target is a <button>, which removes the native checkbox semantics/visibility. For accessibility, either keep the checkbox visible (or at least sr-only instead of hidden) and associate it with a <label>, or implement a proper switch/button pattern (role="switch", aria-checked, etc.) so assistive tech can read and toggle the state reliably.
| // Generate a unique ID for this instance to track duplicates | ||
| const instanceId = Math.random().toString(36).substring(2, 9) | ||
|
|
||
| // Debug props received | ||
| useEffect(() => { | ||
| if (playerRef.current && "remote" in playerRef.current) { | ||
| playerRef.current.remote.watchAvailability(a => { | ||
| console.debug("Cast support: ", a); | ||
| setCanCast(a); | ||
| }); | ||
| console.debug(`LiveVideoPlayer [${instanceId}] Props received:`, { title, stream, status, poster, link }) | ||
| }, [instanceId, title, stream, status, poster, link]) |
There was a problem hiding this comment.
instanceId is generated with Math.random() on every render, so it changes each render and forces the debug/cast useEffects to re-run continuously (and can create repeated subscriptions/log spam). Persist it via useRef/useId so it stays stable for the component lifetime, or remove it entirely once debugging is done.
| useEffect(() => { | ||
| if (playerRef.current && "remote" in playerRef.current) { | ||
| playerRef.current.remote.watchAvailability(a => { | ||
| console.debug("Cast support: ", a); | ||
| setCanCast(a); | ||
| }); | ||
| console.debug(`LiveVideoPlayer [${instanceId}] Props received:`, { title, stream, status, poster, link }) | ||
| }, [instanceId, title, stream, status, poster, link]) |
There was a problem hiding this comment.
Multiple console.debug logs were added here. This will be noisy in production (and in some environments can be a perf/operational concern). Please gate these logs behind a dev/debug flag or remove them before merging.
| <div | ||
| style={{ | ||
| position: 'absolute', | ||
| top: 10, | ||
| left: 10, |
There was a problem hiding this comment.
The HLS diagnostic overlay is always rendered (debug-only UI) and uses position: 'absolute' without ensuring a positioned ancestor, so it can overlay unrelated parts of the page. Please remove it or guard it behind a debug flag and render it with correct positioning.
| } else if (link.type === NostrPrefix.Address || link.type === NostrPrefix.Event) { | ||
| const authorTag = link.tags.find(t => t[0] === 'p') | ||
| if (authorTag) { | ||
| rb.withFilter() | ||
| .kinds([LIVE_STREAM_CHAT, LIVE_STREAM_RAID, LIVE_STREAM_CLIP]) | ||
| .authors([authorTag[1]]) | ||
| .limit(200) | ||
| .relay(relays) | ||
| rb.withFilter().kinds([EventKind.ZapReceipt]).authors([authorTag[1]]).relay(relays) | ||
| } else { | ||
| rb.withFilter().kinds([LIVE_STREAM_CHAT, LIVE_STREAM_RAID, LIVE_STREAM_CLIP]).limit(200).relay(relays) | ||
| rb.withFilter().kinds([EventKind.ZapReceipt]).limit(200).relay(relays) | ||
| } |
There was a problem hiding this comment.
The else if (link.type === NostrPrefix.Address || link.type === NostrPrefix.Event) branch appears unreachable: eventLink is set to link for Address/Event types in the eventLink useMemo, so the earlier if (eventLink) branch will always run instead. Consider removing this dead code or adjusting the eventLink logic so this branch can actually be hit.
| } else if (link.type === NostrPrefix.Address || link.type === NostrPrefix.Event) { | |
| const authorTag = link.tags.find(t => t[0] === 'p') | |
| if (authorTag) { | |
| rb.withFilter() | |
| .kinds([LIVE_STREAM_CHAT, LIVE_STREAM_RAID, LIVE_STREAM_CLIP]) | |
| .authors([authorTag[1]]) | |
| .limit(200) | |
| .relay(relays) | |
| rb.withFilter().kinds([EventKind.ZapReceipt]).authors([authorTag[1]]).relay(relays) | |
| } else { | |
| rb.withFilter().kinds([LIVE_STREAM_CHAT, LIVE_STREAM_RAID, LIVE_STREAM_CLIP]).limit(200).relay(relays) | |
| rb.withFilter().kinds([EventKind.ZapReceipt]).limit(200).relay(relays) | |
| } |
Fixes for stream chat loading issues