Skip to content

Adds rudimentary video support #49

Open
mennoliefstingh wants to merge 9 commits into
ThijsRay:mainfrom
mennoliefstingh:main
Open

Adds rudimentary video support #49
mennoliefstingh wants to merge 9 commits into
ThijsRay:mainfrom
mennoliefstingh:main

Conversation

@mennoliefstingh

Copy link
Copy Markdown

Pull request type

  • 🪳 Bugfix
  • 👮🏻‍♂️ Security/dependency update
  • ✨ Feature (e.g., new script)
  • 📝 Code style update (formatting, renaming)
  • 🏗️ Refactoring (no functional changes, no API changes)
  • 🔨 Build related changes (build scripts, build configs, etc.)
  • 📘 Documentation

Pull request description

Adds video support to the Thijstool in a very rudimentary way by manually recreating the URL from the episode ID and placing that in the description. Adds three new env vars:

  • ENABLE_VIDEO enables this (pre-alpha) functionality
  • ENABLE_VIDEO_CHECK enables functionality that makes a get request to the video url, only adding/marking if a video exists there
  • VIDEO_TITLE_SUFFIX is a suffix that's added to podcast titles iff ENABLE_VIDEO and ENABLE_VIDEO_CHECK and the video URL refers to an actual video.

@dylanvdbrink

Copy link
Copy Markdown

Is something blocking this PR to go through? Would really like to use this as well!

@izu-x

izu-x commented Apr 17, 2026

Copy link
Copy Markdown

Tested this locally and it works great. Found two issues and fixed them:

1. Missing branch when VIDEO_ENABLED=true and VIDEO_CHECK_ENABLED=false

The original code only sets title and description inside the VIDEO_CHECK_ENABLED block. When checking is disabled, those fields never get set and the feed entry ends up empty.

Fix: add an else that sets the URL (unverified) in the description without the title suffix, since we have no confirmation video exists.

2. video_exists_at_url blocks the event loop

The function used requests.get (sync) inside an async context. Under load this stalls all concurrent episode fetches.

Fix: convert to async def using aiohttp with a 10s timeout, same session library already used elsewhere in the codebase. Also drops the requests import which is now unused.

Diff is straightforward, happy to open a PR against this branch if useful.

@mennoliefstingh

Copy link
Copy Markdown
Author

@dylanvdbrink I'd been testing this for a while and found it good-enough but not great as it sometimes doesn't detect correctly.

@izu-x I would appreciate a PR against this branch! I think I have @ThijsRay's contact info somewhere (if he doesn't see this tag) so we could (re-)consider adding video support if it does end up being stable.

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