Conversation
[config/config.example.yml] - Introduce new default value for hide_shorts_and_live [locales/en-US.json] - Add translation label for hide_shorts_and_live [src/invidious/config.cr] - Add default value for hide_shorts_and_live to ConfigPreferences [src/invidious/users.cr] - Add filtering for shorts and listreams if hide_shorts_and_live is active [src/invidious/routes/preferences.cr] - Retrieve and convert hide_shorts_and_live option from preferences page to preferences object [src/invidious/user/preferences.cr] - Add hide_shorts_and_live property [src/invidious/views/user/preferences.ecr] - Add div, label and input for new hide_shorts_and_live option
[src/invidious/users.cr] - Filter out shorts and livestreams in SQL rather than via an array selector. This way, in the relevant cases, we will query and return "limit" amount of videos, rather than a possible lower amount
There was a problem hiding this comment.
Pull request overview
This PR adds a user preference to hide YouTube Shorts and livestreams from the subscription feed. The feature works by filtering out videos with length_seconds = 0, which the existing channel fetching mechanism uses to mark non-regular videos (shorts/livestreams) that don't appear in a channel's video tab.
Changes:
- Added
hide_shorts_and_liveboolean preference across the configuration, user preferences, and routes layers - Modified subscription feed queries to filter by
length_seconds > 0when the preference is enabled - Added UI checkbox in preferences page with localized label
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/invidious/config.cr | Added hide_shorts_and_live property to ConfigPreferences struct |
| src/invidious/user/preferences.cr | Added hide_shorts_and_live property to Preferences struct |
| src/invidious/routes/preferences.cr | Added form parameter handling for the new preference |
| src/invidious/views/user/preferences.ecr | Added checkbox input in subscription settings section |
| src/invidious/users.cr | Added conditional SQL filtering based on length_seconds > 0 in 4 query paths |
| locales/en-US.json | Added translation label for the preference |
| config/config.example.yml | Added documentation for the new config option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hi. Unfortunately, this approach isn't valid for the core of Invidious. Your feature relies on a bug currently tracked here: #5143, where the length of certain videos (e.g., Shorts or livestreams) is incorrectly reported as 0. For a robust solution, we'd need a way to reliably identify these videos using the correct metadata, not based on a bug or side-effect. |
|
I see, can you keep this PR open? I'll introduce logic for identifying them correctly. |
|
If you make a vastly different change. I would suggest to either open a new PR or remove your old commit. |
[config/config.example.yml] - Separate hide_shorts_and_live to hide_shorts and hide_livestreams [config/sql/channel_videos.sql] - Introduce enum video_type - Include video_type as new column for channel_videos [locales/en-US.json] - Add labels for new settings [src/invidious/channels/channels.cr] - Add property video_type of type VideoType to ChannelVideo struct - Add deserializer module for conversion from database entry to enum - Add check if we already have a video in the database. If the video `updated` field has no been updated, only update views - Add check whether a video is in the `videos` array. If this is not the case, fetch the individual video for `video_type` as well as `length_videos` [src/invidious/config.cr] - Separate hide_shorts_and_live property to hide_shorts and hide_livestreams properties [src/invidious/database/channels.cr] - Include video_type in database insert for ChannelVideo [src/invidious/routes/preferences.cr] - Separate hide_shorts_and_live setting to hide_shorts and hide_livestreams [src/invidious/users.cr] - Accumulate VideoTypes in an array and query on these types - Remove paths for hide_shorts_and_live [src/invidious/videos.cr] - Add `Short` entry to VideoType enum [src/invidious/videos/parser.cr] - Add check whether a video is a short
…dious into hide-shorts-and-livestreams
|
Can you check if this approach is valid? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else | ||
| video_type = VideoType::Video | ||
| length_seconds = channel_video.try &.length_seconds | ||
| live_now = channel_video.try &.badges.live_now? | ||
| end |
There was a problem hiding this comment.
When processing regular videos (the else branch starting at line 246), the premiere_timestamp variable is not set. This means it will be uninitialized when used at line 264, which could lead to a compilation error or unexpected behavior. The premiere_timestamp should be set to nil or retrieved from channel_video.premiere_timestamp in the else branch.
There was a problem hiding this comment.
Unsure how default nullability works in crystal.
| CREATE TYPE public.video_type AS ENUM | ||
| ( | ||
| 'Video', | ||
| 'Short', | ||
| 'Livestream', | ||
| 'Scheduled' | ||
| ); |
There was a problem hiding this comment.
This PR adds a new enum type and column to the database schema, but does not provide a migration script for existing databases. Users upgrading to this version will need to manually run this SQL to add the enum type and column. A migration script should be added to config/migrate-scripts/ (following the pattern of existing migration scripts like migrate-db-1eca969.sh) that:
- Creates the enum type if it doesn't exist
- Adds the video_type column to the channel_videos table
- Sets a default value (e.g., 'Video') for existing rows
This is critical for production deployments where the database already contains data.
| [VideoType::Livestream, VideoType::Scheduled].each { |v| types_to_fetch.delete(v) } | ||
| end | ||
|
|
||
| types_to_fetch = types_to_fetch.map { |type| "'#{type}'" }.join(", ") |
There was a problem hiding this comment.
This code constructs SQL by interpolating enum values into the query string. While the values come from a controlled enum (VideoType), this pattern is vulnerable if the enum implementation changes. A safer approach would be to use parameterized queries with an IN clause using an array parameter, or to convert the enum values to their PostgreSQL enum representation properly. Consider using a parameterized query like: "WHERE video_type = ANY($1)" with types_to_fetch as an array parameter.
There was a problem hiding this comment.
Following examples from other queries in the codebase.
| live_now boolean, | ||
| premiere_timestamp timestamp with time zone, | ||
| views bigint, | ||
| video_type video_type, |
There was a problem hiding this comment.
The database column is added to the table schema, but it's not marked as NOT NULL and doesn't have a default value. This could cause issues when inserting videos without explicitly setting video_type. Consider adding a default value in the SQL schema (e.g., DEFAULT 'Video') or ensuring that all code paths that create ChannelVideo objects always set video_type explicitly. The struct has a default value (VideoType::Video), but this may not be respected at the database level.
| video_type video_type, | |
| video_type video_type NOT NULL DEFAULT 'Video', |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Will check the code next week. |
|
@Harm133 Please explain your new changes or change your PR description for a better review of your PR. Thank you. |
Updated the description |
This PR adds two settings for the subscriptions feeds, one to hide livestreams and one to hide shorts. To facilitate this, extra plumbing and a database change is required.
Currently
channel_videosare retrieved via 2 ways, pubsub and theRefreshChannelsJob.The pubsub method (https://github.com/iv-org/invidious/blob/master/src/invidious/routes/feeds.cr#L399?target=https://github.com) gets a notification from the
PubSubHubbuband fetches the video via the companion, extracts/parses the info, and inserts it into thechannel_videostable.The
RefreshChannelsJobwhich calls (https://github.com/iv-org/invidious/blob/master/src/invidious/channels/channels.cr#L158?target=https://github.com), on a set interval, fetches the RSS feeds for all subscribed channel and parses this data and inserts it into thechannel_videostable.The RSS feed information is limited, information for livestreams and shorts are not present (length etc).
Changes are made in the
fetch_channelfunction to use the same logic the pubsub method uses to extract more info from a video.New
fetch_channellogic:youtubeiAPI for the first page of the videos tab of the channel.updatedtimestamp equals that in the database, we only update the view count.youtubeiresult, if this is the case we can say this is a video and parse as we did before.youtubeiresult, we fetch the individual video via (https://github.com/iv-org/invidious/blob/master/src/invidious/videos.cr#L326?target=https://github.com) and continue with the information provided via this call.The pubsub and
RefreshChannelsJob(in case of non-standard videos) both now use (https://github.com/iv-org/invidious/blob/master/src/invidious/videos/parser.cr#L55?target=https://github.com) to extract information, now also determining the videotype (a short is denoted byisShortsEligible)Other changes are made to preferences and the internal fetching of a users subscription feed.
One other change is the inclusion of
Shortin theVideoTypeenum in (https://github.com/iv-org/invidious/blob/master/src/invidious/videos.cr#L1?target=https://github.com)What to discuss:
[ ] Database changes
[ ] Possible database migrations (Nuking all channel_videos, re-fetching all videos, labeling all as video, etc..)
Closes #5143
(Partially) closes #2585