Summary: show airs time and network, episode network#1781
Summary: show airs time and network, episode network#1781
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by providing immediate and comprehensive information about TV shows. It introduces the capability to display a show's air schedule, intelligently converted to the user's local timezone, and its broadcasting network directly on the summary page. This is achieved by extending the data model for show entries and implementing a new utility for timezone conversions, ensuring that users always see relevant and localized scheduling details. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene Coverage  |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Mar 12, 2026 7:49p.m. | Review ↗ |
| Test coverage | |
Mar 12, 2026 7:49p.m. | Review ↗ |
Code Coverage Summary
| Language | Line Coverage (Overall) |
|---|---|
| Aggregate | 27.3% [▼ down 0.1% from main] |
| Javascript | 27.3% [▼ down 0.1% from main] |
➟ Additional coverage metrics may have been reported. See full coverage report ↗
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully integrates the display of TV show air times and network information across the application. The changes include extending the ShowEntry data model, updating the mapping logic, and enhancing the SummaryTitle and MediaDetails components to render this new information. A new utility function, toShowAirs, has been introduced to handle complex timezone conversions, ensuring accurate display of air times in the user's local timezone. The overall implementation is clean and adheres to the functional programming principles outlined in the style guide.
Style Guide Adherence Summary:
- Naming Conventions (ALL_CAPS for Constants): Two instances were found where constants were not named in
ALL_CAPSas required by the repository style guide (line 52). These have been highlighted in the review comments.
Overall, the changes are well-implemented and improve the user experience by providing more comprehensive show details.
| airs: show.airs?.day && show.airs?.time && show.airs?.timezone | ||
| ? { day: show.airs.day, time: show.airs.time, timezone: show.airs.timezone } | ||
| : undefined, |
There was a problem hiding this comment.
The conditional check show.airs?.day && show.airs?.time && show.airs?.timezone is redundant here. Since ShowAirsSchema defines day, time, and timezone as required strings, if show.airs exists, these properties are expected to be present. If show.airs itself is nullish, the optional chaining ?. will correctly propagate undefined.
Simplifying this to show.airs ?? undefined will achieve the same result more concisely, aligning with how network is handled.
airs: show.airs ?? undefined,References
- Validate data received from an API at the source (e.g., in the data-fetching function using schema validation) rather than at every consumption point to ensure robustness and avoid repetition. The
ShowAirsSchemashould guarantee the presence of these properties ifshow.airsexists, making these checks redundant.
| const DAYS_INDEX: Record<string, number> = { | ||
| 'Sunday': 0, | ||
| 'Monday': 1, | ||
| 'Tuesday': 2, | ||
| 'Wednesday': 3, | ||
| 'Thursday': 4, | ||
| 'Friday': 5, | ||
| 'Saturday': 6, | ||
| }; |
There was a problem hiding this comment.
According to the repository's naming conventions (line 52), constants should be named in ALL_CAPS.
Please rename DAYS_INDEX to DAYS_INDEX to adhere to this guideline.
| const DAYS_INDEX: Record<string, number> = { | |
| 'Sunday': 0, | |
| 'Monday': 1, | |
| 'Tuesday': 2, | |
| 'Wednesday': 3, | |
| 'Thursday': 4, | |
| 'Friday': 5, | |
| 'Saturday': 6, | |
| }; | |
| const DAYS_INDEX: Record<string, number> = { | |
| "Sunday": 0, | |
| "Monday": 1, | |
| "Tuesday": 2, | |
| "Wednesday": 3, | |
| "Thursday": 4, | |
| "Friday": 5, | |
| "Saturday": 6, | |
| }; |
References
- Constants should be named in
ALL_CAPS. (link)
rectifyer
force-pushed
the
feat/show_airs_info
branch
from
fafc3ca to
462c6a7
Compare
| airs: show.airs ?? undefined, | ||
| network: show.network ?? undefined, |
There was a problem hiding this comment.
These ?? undefined can go, the schema is already defined as nullish?
| day: z.string(), | ||
| time: z.string(), | ||
| timezone: z.string(), |
There was a problem hiding this comment.
In the response, all of these can also be nullish. You can also check typing issues locally with deno task check ;)
There was a problem hiding this comment.
Oooo, or we can handle it in the mapper. Only set it if all values are in the response? 🤔
rectifyer
force-pushed
the
feat/show_airs_info
branch
2 times, most recently
from
ee087db to
1e0ff51
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds functionality to display a show's airing schedule and network. The changes are well-structured, extending the data models and components logically. I've identified a couple of areas for improvement. First, in SummaryTitle.svelte, the use of new Date() within a $derived computation can lead to stale data; a reactive clock would be a better approach, aligning with the rule for refactoring functions relying on new Date(). Second, the new toHumanDayTime utility function, while clever, is overly complex and could be fragile. I recommend refactoring it to use a dedicated timezone library for better robustness and maintainability. My detailed comments provide more context and suggestions.
| export function toHumanDayTime( | ||
| { day, time, timezone }: ToHumanDayTimeProps, | ||
| locale: string, | ||
| ): { day: string; time: string } | undefined { | ||
| const dayIndex = DAYS_INDEX[day]; | ||
| if (dayIndex == null) { | ||
| return; | ||
| } | ||
|
|
||
| const [hour, minute] = time.split(':').map(Number); | ||
| if (hour == null || minute == null) { | ||
| return; | ||
| } | ||
|
|
||
| // Use a reference week (Jan 5, 2025 = Sunday) to create a date for the target day. | ||
| // We set the time in UTC first, then calculate the offset to the source timezone | ||
| // so we can find the true UTC moment. | ||
| const refDateMs = Date.UTC(2025, 0, 5 + dayIndex, hour, minute); | ||
|
|
||
| // Find what time our UTC guess displays as in the source timezone | ||
| const tzParts = new Intl.DateTimeFormat('en-US', { | ||
| timeZone: timezone, | ||
| hour: 'numeric', | ||
| minute: 'numeric', | ||
| day: 'numeric', | ||
| hour12: false, | ||
| }).formatToParts(new Date(refDateMs)); | ||
|
|
||
| const tzHour = Number( | ||
| tzParts.find((p) => p.type === 'hour')?.value ?? 0, | ||
| ); | ||
| const tzMinute = Number( | ||
| tzParts.find((p) => p.type === 'minute')?.value ?? 0, | ||
| ); | ||
| const tzDay = Number( | ||
| tzParts.find((p) => p.type === 'day')?.value ?? 0, | ||
| ); | ||
|
|
||
| // Calculate offset between what we want in the source tz vs what we got | ||
| const refDay = 5 + dayIndex; | ||
| const dayDiffMinutes = (refDay - tzDay) * 24 * 60; | ||
| const timeDiffMinutes = (hour * 60 + minute) - (tzHour * 60 + tzMinute); | ||
| const totalOffsetMs = (dayDiffMinutes + timeDiffMinutes) * 60 * 1000; | ||
|
|
||
| // The actual UTC moment | ||
| const utcDate = new Date(refDateMs + totalOffsetMs); | ||
|
|
||
| // Format in the user's local timezone | ||
| const dayStr = new Intl.DateTimeFormat(locale, { | ||
| weekday: 'long', | ||
| }).format(utcDate); | ||
|
|
||
| const timeStr = new Intl.DateTimeFormat(locale, { | ||
| hour: 'numeric', | ||
| minute: '2-digit', | ||
| }).format(utcDate); | ||
|
|
||
| return { day: dayStr, time: timeStr }; | ||
| } |
There was a problem hiding this comment.
This function for timezone conversion is quite complex and reimplements logic that is prone to errors, especially with edge cases like Daylight Saving Time. This makes it hard to read, maintain, and verify for correctness, violating the style guide's principles of simplicity and readability (lines 24, 28).
A more robust approach would be to use a dedicated date/timezone library like date-fns-tz, which is designed to handle these complexities.
Additionally, the parsing of date parts from Intl.DateTimeFormat on lines 52-60 could be made safer with guard clauses to ensure all parts are found before being used in calculations, as recommended by the style guide (line 88). The current use of ?? 0 can mask errors and lead to incorrect offsets if a part is unexpectedly missing.
Consider this safer parsing approach:
const tzHourStr = tzParts.find((p) => p.type === 'hour')?.value;
const tzMinuteStr = tzParts.find((p) => p.type === 'minute')?.value;
const tzDayStr = tzParts.find((p) => p.type === 'day')?.value;
if (tzHourStr === undefined || tzMinuteStr === undefined || tzDayStr === undefined) {
return; // Or handle error appropriately
}
const tzHour = Number(tzHourStr);
const tzMinute = Number(tzMinuteStr);
const tzDay = Number(tzDayStr);References
- The current implementation is complex and not easily readable, which goes against the principle of favoring readability and simplicity over cleverness (Lines 24, 28). (link)
- The function could benefit from more guard clauses to handle cases where date parts might not be found, which aligns with the 'Early Exits' principle (Line 88). (link)
There was a problem hiding this comment.
@gemini-code-assist, what about instead of using custom logic, we used date-fns-tz?
There was a problem hiding this comment.
Yes, that's exactly what I was suggesting! Using a library like date-fns-tz would be a significant improvement. It's specifically designed to handle the complexities of timezones, including Daylight Saving Time and other edge cases, which custom logic often struggles with. This would greatly reduce the complexity and potential for errors in the toHumanDayTime function, making it more robust and easier to maintain. It aligns with the principle of leveraging well-tested external libraries for complex tasks rather than reimplementing them.
There was a problem hiding this comment.
@gemini-code-assist, sorry, my bad, I left my comment before having read yours 😅
| const label = target.media.airDate > new Date() | ||
| ? m.header_expected_premiere() | ||
| : m.header_airs(); |
There was a problem hiding this comment.
The use of new Date() inside a $derived computation makes it impure and can lead to stale UI. The airsText value will not update as time passes. For example, if a show's premiere time is reached while the user is on the page, the label will remain "Expected Premiere" instead of updating to "Airs".
To fix this, you could introduce a reactive clock store that updates periodically (e.g., every minute) and use its value here. This would make your derived state correctly reactive to time changes.
Example of a reactive clock store (e.g., in $lib/stores/clock.ts):
import { readable } from 'svelte/store';
export const now = readable(new Date(), set => {
const interval = setInterval(() => {
set(new Date());
}, 60 * 1000); // Update every minute
return () => clearInterval(interval);
});Then you can import and use $now in your derived computations:
import { now } from '$lib/stores/clock.ts';
// ...
const label = target.media.airDate > $now
? m.header_expected_premiere()
: m.header_airs();This same issue exists for the status derived value on line 22, which also uses new Date().
References
- The style guide (line 17) recommends writing pure functions that avoid side effects. Using
new Date()inside a derived computation is a side effect that makes it impure and dependent on the time of execution rather than only on reactive state. (link) - The style guide (line 231) specifies that
$derived()should be used for computed values that depend on reactive state.new Date()is not a reactive source, which can lead to stale data. (link) - Functions that rely on
new Date()should be refactored to accept the current date as a parameter (dependency injection) to make them pure, deterministic, and easier to test. The suggested reactive store provides a mechanism for this dependency injection in a reactive context.
There was a problem hiding this comment.
@rectifyer, I think for now we can skip interval.
rectifyer
force-pushed
the
feat/show_airs_info
branch
from
1e0ff51 to
74d1419
Compare
|
@seferturan I made several updates based on your comments on the gemini review |
|
@rectifyer, it's starting to get crowded with the labels there 😅 We should maybe only show it if it's on a users favorite service or something? |
|
@seferturan This change is more to know when the show/episode is airing the first time on live TV or a specific streaming network. It's not related to your favorite streaming services. I agree the summary page is too crowded, so I'll request a design review on this. |
|
Maybe the upcoming details drawer could be a good place for it 🤔 I'll also bring it up at the design sync monday :D |
|
@rectifyer, the new details drawer is now in, the air time can be added there: For now, let's remove it from the main summary page until we have a better heuristic on when to show it there cc @ElMagnea |
|
@seferturan This PR has drifted too far from main. I'll close it in favor of #1968 |
2 participants
Description
This pull request introduces the display of a TV show's air schedule (day, time, and timezone) and its network directly on the summary page. This enhancement provides users with more comprehensive and immediately relevant information about their favorite shows. The air schedule is intelligently converted to the user's local timezone, ensuring accuracy and convenience.
To achieve this, the data model for
ShowEntryhas been extended to includeairsandnetworkproperties. A new utility function,toShowAirs, has been implemented to handle the complex timezone conversions, transforming the show's original broadcast time into a localized day and time. This formatted "airs" information is now prominently displayed in theSummaryTitlecomponent for shows. Additionally, the network name is integrated into theMediaDetailscomponent for both shows and episodes, offering a more complete overview.Show, Don't Tell: Screenshots and Videos
Desktop (show summary)
Mobile (show summary)
Mobile (episode summary)
Testing: The Dress Rehearsal
While specific new unit tests were not part of this diff, existing mock data has been updated to include the new
airsandnetworkfields, ensuring the new data structures are correctly handled during development and testing phases.