Improvements to release date sorting#77
Conversation
…aylist that have the same release date, it now sorts them by season and episode number.
Reviewer's GuideEnhances release date sorting by introducing utilities to detect episode metadata and updating both ascending and descending sort orders to group items by release date and tie-break using episode status, season number, and episode number. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@jyourstone has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe change adds three public static methods to OrderUtilities: GetSeasonNumber(BaseItem), GetEpisodeNumber(BaseItem), and IsEpisode(BaseItem). ReleaseDateOrder.OrderBy now groups items by release date and sorts groups ascending; within each group, items are ordered with episodes first, then by season number, then episode number. ReleaseDateOrderDesc.OrderBy similarly groups by release date but sorts groups descending; within groups, episodes come first, then season and episode numbers in descending order. Reflection retrieves index values; null collections are handled. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs:1580` </location>
<code_context>
+ /// </summary>
+ /// <param name="item">The BaseItem to check</param>
+ /// <returns>True if the item is an episode, false otherwise</returns>
+ public static bool IsEpisode(BaseItem item)
+ {
+ return item?.GetType().Name == "Episode";
</code_context>
<issue_to_address>
Type name comparison may be fragile if class names change.
Using string comparison for type checking can break if the class name changes or if inheritance is involved. Prefer 'is Episode' or direct type comparison for reliability.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
public static bool IsEpisode(BaseItem item)
{
return item?.GetType().Name == "Episode";
}
=======
public static bool IsEpisode(BaseItem item)
{
return item is Episode;
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public static bool IsEpisode(BaseItem item) | ||
| { | ||
| return item?.GetType().Name == "Episode"; | ||
| } |
There was a problem hiding this comment.
suggestion: Type name comparison may be fragile if class names change.
Using string comparison for type checking can break if the class name changes or if inheritance is involved. Prefer 'is Episode' or direct type comparison for reliability.
| public static bool IsEpisode(BaseItem item) | |
| { | |
| return item?.GetType().Name == "Episode"; | |
| } | |
| public static bool IsEpisode(BaseItem item) | |
| { | |
| return item is Episode; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs (5)
1515-1543: Avoid reflection in hot path: use typed Episode properties for season numberReflection here is unnecessary and will be called repeatedly during sorting. Strongly-typed access is faster and simpler.
Apply this diff to replace reflection with a type check and direct property access:
- public static int GetSeasonNumber(BaseItem item) - { - try - { - var parentIndexProperty = item.GetType().GetProperty("ParentIndexNumber"); - if (parentIndexProperty != null) - { - var value = parentIndexProperty.GetValue(item); - if (value is int intValue) - return intValue; - if (value != null) - { - var nullableValue = value as int?; - if (nullableValue.HasValue) - return nullableValue.Value; - } - } - } - catch - { - // Ignore errors and fall back to 0 - } - return 0; - } + public static int GetSeasonNumber(BaseItem item) + { + return item is MediaBrowser.Controller.Entities.TV.Episode ep + ? (ep.ParentIndexNumber ?? 0) + : 0; + }Additionally, consider making these helpers internal if you don't intend to expose new public API surface.
1545-1573: Same here: drop reflection and read IndexNumber directlyDirect access is faster and clearer; this runs in sort key selectors.
Apply this diff:
- public static int GetEpisodeNumber(BaseItem item) - { - try - { - var indexProperty = item.GetType().GetProperty("IndexNumber"); - if (indexProperty != null) - { - var value = indexProperty.GetValue(item); - if (value is int intValue) - return intValue; - if (value != null) - { - var nullableValue = value as int?; - if (nullableValue.HasValue) - return nullableValue.Value; - } - } - } - catch - { - // Ignore errors and fall back to 0 - } - return 0; - } + public static int GetEpisodeNumber(BaseItem item) + { + return item is MediaBrowser.Controller.Entities.TV.Episode ep + ? (ep.IndexNumber ?? 0) + : 0; + }
1575-1583: Prefer type checks over string name comparisonsString-based type checks are brittle and slower. Use the concrete type check.
Apply this diff:
- public static bool IsEpisode(BaseItem item) - { - return item?.GetType().Name == "Episode"; - } + public static bool IsEpisode(BaseItem item) + { + return item is MediaBrowser.Controller.Entities.TV.Episode; + }
1692-1699: Simplify to a single composite OrderBy and use date-only to group “same release date”Grouping then sorting is more work than necessary and compares full DateTime (time-of-day), which can split items released on the same calendar day. A composite OrderBy chain is simpler and faster, and using .Date matches the PR intent.
Apply this diff:
- // Group by release date, then sort each group properly - return items.GroupBy(OrderUtilities.GetReleaseDate) - .OrderBy(g => g.Key) // Sort date groups ascending - .SelectMany(dateGroup => - dateGroup.OrderBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date - .ThenBy(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0) - .ThenBy(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0) - ); + // Sort by release date (day precision), then within the same day: episodes first, then by SxxExx + return items + .OrderBy(item => OrderUtilities.GetReleaseDate(item).Date) + .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date + .ThenBy(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0) + .ThenBy(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0);Note: If you prefer not to prioritize episodes over non-episodes within the same date, drop the ThenBy that enforces episodes-first and only keep the season/episode ThenBys; non-episodes will retain stable relative order.
1709-1718: Mirror the same composite approach for descending; confirm intended tie-breaksThe descending variant should also switch to a composite OrderBy chain. Also, confirm whether “descending” within the same date should reverse season/episode, or stay ascending (since the date is identical). Current code chooses descending for both.
Apply this diff:
- // Group by release date, then sort each group properly - return items.GroupBy(OrderUtilities.GetReleaseDate) - .OrderByDescending(g => g.Key) // Sort date groups descending - .SelectMany(dateGroup => - dateGroup.OrderBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date - .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0) // Season descending - .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0) // Episode descending - ); + // Sort by release date (day precision) descending; within same day, episodes first then SxxExx + return items + .OrderByDescending(item => OrderUtilities.GetReleaseDate(item).Date) + .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date + .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0) + .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0);Questions to confirm:
- Do we really want “episodes first” within a date group, or only to tie-break episodes relative to each other? The PR summary only mentions ordering episodes among themselves.
- For the Desc variant, should season/episode be descending within the same date? Many users expect S/E to be ascending even when the top-level date is descending. Happy to adjust and send a patch either way.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.cs: For direct property access in C#, use simple property assignments for cheap operations (e.g., Genres, Studios, Tags, Name, ProductionYear).
For expensive operations requiring Library Manager queries, use query objects and reflection to invoke methods (e.g., GetPeople).
For reflection-based discovery in C#, use reflection to enumerate properties and methods for debugging or dynamic access.
When debugging unknown data structures in C#, log the type, properties, and, if enumerable, the first few items and their properties.
BaseItem properties such as Name, ProductionYear, OfficialRating, Genres, Studios, Tags, CommunityRating, CriticRating, DateCreated, DateModified, DateLastRefreshed, and IsPlayed(user) should be accessed directly.
People/Cast/Crew fields require Library Manager queries using InternalPeopleQuery with ItemId.
Audio/Subtitle streams should be accessed via MediaSources property or GetMediaStreams() method.
Related items should be accessed using appropriate query objects.
User data fields (PlayCount, IsFavorite, LastPlayedDate) require UserDataManager.
Artists (track-level) and AlbumArtists (album-level) are separate from People field (movies/TV only) and are cheap operations (simple property access via reflection), not included in expensive field optimization logic.
Files:
Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs
**/SmartPlaylist*.cs
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
For performance optimization, use two-phase filtering for expensive fields (AudioLanguages, People): phase 1 for cheap properties, phase 2 for expensive data only for items passing phase 1.
Files:
Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs
🧬 Code Graph Analysis (1)
Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs (1)
Jellyfin.Plugin.SmartPlaylist/QueryEngine/Factory.cs (1)
BaseItem(689-713)
…ic and improve readability. Update IsEpisode method to use type checking instead of string comparison. Enhance sorting logic in playlist methods to directly order items by release date and episode attributes without grouping.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs:1654` </location>
<code_context>
public override IEnumerable<BaseItem> OrderBy(IEnumerable<BaseItem> items)
{
- return items == null ? [] : items.OrderBy(OrderUtilities.GetReleaseDate);
+ if (items == null) return [];
+
+ // Sort by release date (day precision), then within the same day: episodes first, then by season/episode
</code_context>
<issue_to_address>
Returning an empty array for null input is fine, but consider using Array.Empty<BaseItem>() for clarity.
Array.Empty<BaseItem>() is preferable to [] as it avoids extra allocations and enhances code clarity.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (items == null) return [];
=======
if (items == null) return Array.Empty<BaseItem>();
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs:1658` </location>
<code_context>
+
+ // Sort by release date (day precision), then within the same day: episodes first, then by season/episode
+ return items
+ .OrderBy(item => OrderUtilities.GetReleaseDate(item).Date)
+ .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date
+ .ThenBy(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0)
+ .ThenBy(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0);
</code_context>
<issue_to_address>
The ordering logic prioritizes episodes within the same release date, but may not handle items with missing or default season/episode numbers optimally.
If season or episode numbers default to 0, specials or items with missing data may be sorted before regular episodes. Consider refining the logic to better handle these cases.
Suggested implementation:
```csharp
// Sort by release date (day precision), then within the same day: episodes first, then by season/episode
return items
.OrderBy(item => OrderUtilities.GetReleaseDate(item).Date)
.ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date
.ThenBy(item => OrderUtilities.IsEpisode(item) ? GetSortableSeasonNumber(item) : int.MaxValue)
.ThenBy(item => OrderUtilities.IsEpisode(item) ? GetSortableEpisodeNumber(item) : int.MaxValue);
```
```csharp
/// <summary>
/// Returns a sortable season number, putting specials or missing data last.
/// </summary>
private static int GetSortableSeasonNumber(BaseItem item)
{
var season = OrderUtilities.GetSeasonNumber(item);
// Treat 0 or negative as missing/special, sort them last
return (season > 0) ? season : int.MaxValue;
}
/// <summary>
/// Returns a sortable episode number, putting specials or missing data last.
/// </summary>
private static int GetSortableEpisodeNumber(BaseItem item)
{
var episode = OrderUtilities.GetEpisodeNumber(item);
// Treat 0 or negative as missing/special, sort them last
return (episode > 0) ? episode : int.MaxValue;
}
```
</issue_to_address>
### Comment 3
<location> `Jellyfin.Plugin.SmartPlaylist/SmartPlaylist.cs:1675` </location>
<code_context>
+
+ // Sort by release date (day precision) descending; within same day, episodes first then season/episode descending
+ return items
+ .OrderByDescending(item => OrderUtilities.GetReleaseDate(item).Date)
+ .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date
+ .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0)
+ .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0);
</code_context>
<issue_to_address>
Descending order logic may result in specials or items with missing season/episode numbers appearing before regular episodes.
Consider refining the sorting logic to ensure items with missing or default season/episode numbers are placed appropriately relative to regular episodes.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Sort by release date (day precision) descending; within same day, episodes first then season/episode descending
return items
.OrderByDescending(item => OrderUtilities.GetReleaseDate(item).Date)
.ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date
.ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0)
.ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0);
=======
// Sort by release date (day precision) descending; within same day, episodes first, regular episodes before specials/missing, then season/episode descending
return items
.OrderByDescending(item => OrderUtilities.GetReleaseDate(item).Date)
.ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date
.ThenBy(item =>
OrderUtilities.IsEpisode(item)
? (OrderUtilities.GetSeasonNumber(item) > 0 && OrderUtilities.GetEpisodeNumber(item) > 0 ? 0 : 1)
: 2
) // Regular episodes before specials/missing
.ThenByDescending(item => OrderUtilities.IsEpisode(item) && OrderUtilities.GetSeasonNumber(item) > 0 ? OrderUtilities.GetSeasonNumber(item) : int.MinValue)
.ThenByDescending(item => OrderUtilities.IsEpisode(item) && OrderUtilities.GetEpisodeNumber(item) > 0 ? OrderUtilities.GetEpisodeNumber(item) : int.MinValue);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public override IEnumerable<BaseItem> OrderBy(IEnumerable<BaseItem> items) | ||
| { | ||
| return items == null ? [] : items.OrderBy(OrderUtilities.GetReleaseDate); | ||
| if (items == null) return []; |
There was a problem hiding this comment.
suggestion: Returning an empty array for null input is fine, but consider using Array.Empty() for clarity.
Array.Empty() is preferable to [] as it avoids extra allocations and enhances code clarity.
| if (items == null) return []; | |
| if (items == null) return Array.Empty<BaseItem>(); |
| .OrderBy(item => OrderUtilities.GetReleaseDate(item).Date) | ||
| .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date |
There was a problem hiding this comment.
suggestion: The ordering logic prioritizes episodes within the same release date, but may not handle items with missing or default season/episode numbers optimally.
If season or episode numbers default to 0, specials or items with missing data may be sorted before regular episodes. Consider refining the logic to better handle these cases.
Suggested implementation:
// Sort by release date (day precision), then within the same day: episodes first, then by season/episode
return items
.OrderBy(item => OrderUtilities.GetReleaseDate(item).Date)
.ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date
.ThenBy(item => OrderUtilities.IsEpisode(item) ? GetSortableSeasonNumber(item) : int.MaxValue)
.ThenBy(item => OrderUtilities.IsEpisode(item) ? GetSortableEpisodeNumber(item) : int.MaxValue); /// <summary>
/// Returns a sortable season number, putting specials or missing data last.
/// </summary>
private static int GetSortableSeasonNumber(BaseItem item)
{
var season = OrderUtilities.GetSeasonNumber(item);
// Treat 0 or negative as missing/special, sort them last
return (season > 0) ? season : int.MaxValue;
}
/// <summary>
/// Returns a sortable episode number, putting specials or missing data last.
/// </summary>
private static int GetSortableEpisodeNumber(BaseItem item)
{
var episode = OrderUtilities.GetEpisodeNumber(item);
// Treat 0 or negative as missing/special, sort them last
return (episode > 0) ? episode : int.MaxValue;
}| // Sort by release date (day precision) descending; within same day, episodes first then season/episode descending | ||
| return items | ||
| .OrderByDescending(item => OrderUtilities.GetReleaseDate(item).Date) | ||
| .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date | ||
| .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0) | ||
| .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0); |
There was a problem hiding this comment.
suggestion: Descending order logic may result in specials or items with missing season/episode numbers appearing before regular episodes.
Consider refining the sorting logic to ensure items with missing or default season/episode numbers are placed appropriately relative to regular episodes.
| // Sort by release date (day precision) descending; within same day, episodes first then season/episode descending | |
| return items | |
| .OrderByDescending(item => OrderUtilities.GetReleaseDate(item).Date) | |
| .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date | |
| .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetSeasonNumber(item) : 0) | |
| .ThenByDescending(item => OrderUtilities.IsEpisode(item) ? OrderUtilities.GetEpisodeNumber(item) : 0); | |
| // Sort by release date (day precision) descending; within same day, episodes first, regular episodes before specials/missing, then season/episode descending | |
| return items | |
| .OrderByDescending(item => OrderUtilities.GetReleaseDate(item).Date) | |
| .ThenBy(item => OrderUtilities.IsEpisode(item) ? 0 : 1) // Episodes first within same date | |
| .ThenBy(item => | |
| OrderUtilities.IsEpisode(item) | |
| ? (OrderUtilities.GetSeasonNumber(item) > 0 && OrderUtilities.GetEpisodeNumber(item) > 0 ? 0 : 1) | |
| : 2 | |
| ) // Regular episodes before specials/missing | |
| .ThenByDescending(item => OrderUtilities.IsEpisode(item) && OrderUtilities.GetSeasonNumber(item) > 0 ? OrderUtilities.GetSeasonNumber(item) : int.MinValue) | |
| .ThenByDescending(item => OrderUtilities.IsEpisode(item) && OrderUtilities.GetEpisodeNumber(item) > 0 ? OrderUtilities.GetEpisodeNumber(item) : int.MinValue); |
1 participant
If there are episodes in the playlist that have the same release date, it now sorts them by season and episode number.
Summary by Sourcery
Improve release date ordering by adding helper methods for episode metadata and refining sorting to group episodes first and sort them by season and episode numbers when dates are identical.
Enhancements: