Skip to content

fix(live-news): call destroyPlayer() before offline/embed error message (fixes #347)#388

Closed
Niboshi-Wasabi wants to merge 4 commits intokoala73:mainfrom
Niboshi-Wasabi:fix/issue-347-live-news-black-screen
Closed

fix(live-news): call destroyPlayer() before offline/embed error message (fixes #347)#388
Niboshi-Wasabi wants to merge 4 commits intokoala73:mainfrom
Niboshi-Wasabi:fix/issue-347-live-news-black-screen

Conversation

@Niboshi-Wasabi
Copy link
Contributor

@Niboshi-Wasabi Niboshi-Wasabi commented Feb 26, 2026

Summary

1. Live News panel black screen (Issue #347)
Fixes the bug where the Live News panel stays black after switching from an offline channel back to a live channel. The root cause was that showOfflineMessage() and showEmbedError() replaced the panel content without calling destroyPlayer(), so this.player remained set. When switching back to a live channel, initializePlayer() returned early and no new YouTube player was created. This PR calls destroyPlayer() at the start of both methods so that a new player is created when returning to a live stream.

2. canvas-confetti import resolution
Resolves the Vite error Failed to resolve import "canvas-confetti" when the package is missing from node_modules (e.g. fresh clone). The celebration service now loads confetti at runtime via a CDN script instead of bundling the package, so the app builds and runs without requiring canvas-confetti to be installed. vite.config.ts includes optimizeDeps.include: ['canvas-confetti'] for environments where the package is present.

Type of change

  • Bug fix
  • New feature
  • New data source / feed
  • New map layer
  • Refactor / code cleanup
  • Documentation
  • CI / Build / Infrastructure

Affected areas

  • Map / Globe
  • News panels / RSS feeds
  • AI Insights / World Brief
  • Market Radar / Crypto
  • Desktop app (Tauri)
  • API endpoints (/api/*)
  • Config / Settings
  • Other: Live News panel (YouTube embed), celebration service & Vite config

Checklist

  • Tested on worldmonitor.app variant
  • Tested on tech.worldmonitor.app variant (if applicable)
  • New RSS feed domains added to api/rss-proxy.js allowlist (if adding feeds)
  • No API keys or secrets committed
  • TypeScript compiles without errors (npm run typecheck)

Screenshots

No screenshots. #347: Reproduce by adding an offline channel → switch to it → switch back to a live channel; the stream now loads instead of staying black. Confetti: App builds and runs without canvas-confetti in node_modules; celebrations load confetti from CDN when used.

@vercel
Copy link

vercel bot commented Feb 26, 2026

@Niboshi-Wasabi is attempting to deploy a commit to the Elie Team on Vercel.

A member of the Team first needs to authorize it.

@koala73
Copy link
Owner

koala73 commented Feb 26, 2026

Thanks for this PR @Niboshi-Wasabi! We reviewed both fixes:

Fix 1: Live News black screen — ✅ Looks great

The root cause analysis is spot on. showOfflineMessage() and showEmbedError() replace this.content.innerHTML without cleaning up this.player, so initializePlayer() returns early on the next channel switch → black screen. Adding this.destroyPlayer() at the top of both methods is the right fix — it's surgical and destroyPlayer() is already idempotent so the onError path (which calls destroyPlayer() then showEmbedError()) won't double-destroy.

We'll take this fix and merge it separately so users benefit from it immediately.

Fix 2: canvas-confetti CDN loading — Needs rework

A few issues we found:

  1. Missing promise cacheloadConfetti() appends a new <script> tag on every call until the first one finishes loading. The record celebration calls run() twice 300ms apart — if the script hasn't loaded in 300ms, a duplicate <script> tag is appended. Needs a module-scoped cached promise.

  2. optimizeDeps.include: ['canvas-confetti'] is contradictory — This tells Vite to pre-bundle canvas-confetti at dev startup, which will error if the package isn't installed. This contradicts the goal of making the package optional.

  3. Unclear rationalecanvas-confetti is already in package.json as a direct dependency. After npm install it resolves fine. Could you describe the specific scenario where the import fails? (CI environment, Tauri build, specific Node version?) That would help us determine whether CDN loading is the right fix vs. ensuring npm install runs correctly.

We recommend splitting this PR: we'll merge Fix 1 now, and if you'd like to submit Fix 2 separately with the promise caching fix and a clearer description of when the error occurs, we're happy to review it.

@Niboshi-Wasabi
Copy link
Contributor Author

Hi @koala73,

Thanks for the review. We’ve reverted Fix 2 (canvas-confetti) from this PR, so the current PR only contains Fix 1 (Live News panel black screen).

On the canvas-confetti import failure: in our case it failed when the package was not present in node_modules — for example right after cloning the repo without running npm install, or in a workspace where dependencies hadn’t been installed yet. Vite then reported “Failed to resolve import 'canvas-confetti'” when starting the dev server or running the build. We didn’t see it in CI, Tauri build, or a specific Node version; it was simply the “package not installed” case.

So we’re fine with the current approach (require npm install so canvas-confetti is available). If you’d prefer to support the “no npm install yet” scenario with optional CDN loading, we can submit a separate PR later with the promise-cache fix and this rationale in the description. Either way works for us.

Thanks again for reviewing.

@koala73
Copy link
Owner

koala73 commented Feb 26, 2026

Code Review

The core fix (LiveNewsPanel.ts) is correct and minimal. Adding this.destroyPlayer() at the start of showOfflineMessage() and showEmbedError() is the right approach — without it, this.player stays set after innerHTML replacement, and initializePlayer() bails early on the next channel switch. Two lines, well-targeted.

Requests before merge:

  1. Remove docs/FIX2_CANVAS_CONFETTI_REWORK.md and docs/PR_347_BODY.md — These are working notes/scratch files that don't belong in the repo. The PR description already documents everything. The confetti rework doc is for a future PR that was reverted. These add 71 lines of non-functional docs noise.

  2. The celebration.ts refactor is unrelated — The third commit reverted the CDN loading but left a run() helper refactor in celebration.ts. This is cosmetic cleanup unrelated to issue Live News panel goes black after switching from offline channel back to live channel #347. The original void confetti(...) calls were clear. Not harmful, but bloats a 2-line bug fix PR. Consider reverting this file entirely to keep the PR focused.

Verdict: Core fix is sound — just remove the two docs files and ideally revert the celebration.ts cosmetic change, then this is good to merge.

… original

- Remove docs/FIX2_CANVAS_CONFETTI_REWORK.md and docs/PR_347_BODY.md (working notes, not for repo)
- Revert celebration.ts to original void confetti(...) calls (no run() helper); keeps PR focused on koala73#347 fix

Made-with: Cursor
@Niboshi-Wasabi
Copy link
Contributor Author

Hi @koala73,

Done. I’ve removed docs/FIX2_CANVAS_CONFETTI_REWORK.md and docs/PR_347_BODY.md, and reverted celebration.ts to the original void confetti(...) calls (no run() helper). The PR is now focused on the #347 fix only. Thanks for the review.

@koala73
Copy link
Owner

koala73 commented Feb 26, 2026

Deployed in #410 (comment)

@koala73 koala73 closed this Feb 26, 2026
@koala73
Copy link
Owner

koala73 commented Feb 26, 2026

thank you @Niboshi-Wasabi

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.

2 participants