Add video support#47
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds HLS video playback by parsing ChangesStarpod video playback, sticky rail, and mobile navigation
Sequence DiagramsequenceDiagram
participant User
participant EpisodeCta
participant Signals as State Signals
participant Player
participant getEpisodeVideos
participant HLSorNative as hls.js / native video
User->>EpisodeCta: click Watch/Play
EpisodeCta->>Signals: set currentEpisode, mediaMode, isPlaying, isTheaterOpen
Signals-->>Player: reactive update
Player->>getEpisodeVideos: episode.video.src available?
alt video mode, no native HLS
Player->>HLSorNative: lazy import hls.js, loadSource(src), attachMedia(videoEl)
else native or audio
Player->>HLSorNative: videoEl.src = src
end
HLSorNative-->>Player: loadedmetadata
Player->>HLSorNative: currentTime = resume pos, play()
HLSorNative-->>Signals: timeupdate → currentTime, play/pause → isPlaying
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/FullPlayButton.tsx (1)
59-68: 🎯 Functional Correctness | 🟡 MinorSet playback mode explicitly in
FullPlayButton
currentEpisodeis updated here, butmediaModeandisTheaterOpenare left as-is. SincemediaModedefaults to'video', list play on an episode with video will start video playback while the theater stays closed, and later clicks inherit whatever mode a previous CTA left behind. SetmediaMode.valuehere (and open the theater for video, or force'audio'if this button should stay audio-only).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/FullPlayButton.tsx` around lines 59 - 68, `FullPlayButton` updates `currentEpisode` and `isPlaying`, but leaves `mediaMode` and `isTheaterOpen` stale, so playback can inherit the previous CTA state. In the `FullPlayButton` click flow where `currentEpisode.value` is assigned, explicitly set `mediaMode.value` to the intended mode for this button and update `isTheaterOpen.value` accordingly when starting video playback, or force audio mode if this action should remain audio-only. Keep the behavior localized to this handler so future clicks always start from a predictable state.
🧹 Nitpick comments (3)
src/components/EpisodeCta.tsx (1)
100-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor consistency nits.
- Line 100:
becomingCurrent ? true : falseis justbecomingCurrent.- Line 116 casts the icon
as JSX.Elementbut the pill variant (Line 126) does not. Align them or drop the cast.Also applies to: 116-116, 126-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/EpisodeCta.tsx` at line 100, Simplify the conditional assignment in EpisodeCta so isPlaying.value uses becomingCurrent directly instead of a ternary that returns true/false. Also make the icon rendering consistent between the two branches by either removing the unnecessary JSX.Element cast on the icon in the current branch or adding the same cast in the pill variant branch, using the existing render logic around EpisodeCta and its icon/pill JSX.src/pages/[episode].astro (1)
170-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCTA wiring looks correct; consider extracting the repeated
episodeliteral.The same
episode={{ audio, video, episodeImage, episodeNumber, id, title }}object is duplicated four times. Hoisting it into a single frontmatter const (e.g.const ctaEpisode = {...}) would remove the repetition and keep the field list in sync.Also applies to: 204-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/`[episode].astro around lines 170 - 183, The EpisodeCta usage is repeating the same episode object shape in multiple places, so hoist that literal into a single frontmatter constant in [episode].astro and reuse it wherever EpisodeCta is rendered. Update the repeated EpisodeCta calls to reference the shared object (using the existing audio, video, episodeImage, episodeNumber, id, and title fields) so the field list stays synchronized across all instances.src/lib/rss.ts (1)
101-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHLS type match is case-sensitive.
alt['@_type'] === HLS_TYPErequires an exactapplication/x-mpegURL. Feeds commonly useapplication/x-mpegurl(lowercase) orapplication/vnd.apple.mpegurl, which would be missed. A case-insensitive compare is more robust.♻️ Proposed change
const hls = alternates.find( - (alt: Record<string, unknown>) => alt['`@_type`'] === HLS_TYPE + (alt: Record<string, unknown>) => + String(alt['`@_type`']).toLowerCase() === HLS_TYPE.toLowerCase() );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/rss.ts` around lines 101 - 104, The HLS alternate lookup in rss.ts is too strict because the @'_type' comparison in the alternates.find logic only matches one exact case/form. Update the HLS_TYPE matching in the relevant rss parsing flow to do a case-insensitive comparison and accept common variants like application/x-mpegurl and application/vnd.apple.mpegurl, while keeping the existing hls selection behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.impeccable/briefs/reshape-sticky-rail.md:
- Around line 145-147: The config-validation guidance is inconsistent with the
repository standard: update the `defineStarpodConfig` guidance for the new `cta`
field to recommend Valibot-based validation instead of an
identity-function/default skip. Keep the instructions aligned with the existing
config pattern in `defineStarpodConfig` and `starpod.config.ts`, and remove any
wording that tells implementers not to introduce Valibot in this pass.
In `@DESIGN.md`:
- Around line 295-300: The mobile navigation docs are out of sync with the
shipped layout behavior: the identity rail no longer moves its content into a
footer on mobile, and the architecture uses a mobile slide-over nav instead.
Update the relevant design text around the Identity rail and nav behavior to
describe the actual mobile slide-over pattern and remove any mention of a
mobile-only footer so future changes reference the correct structure.
- Around line 281-283: The focus styling guidance currently tells readers to use
!outline-none, but for Tailwind v4 the intended utility is outline-hidden when
hiding the default outline while preserving forced-colors behavior. Update the
focus indicator guidance in DESIGN.md to reference outline-hidden instead of
!outline-none, keeping the rest of the border-focused behavior described around
the focus state intact.
In `@src/components/Player.tsx`:
- Around line 28-30: The Player component has a Rules-of-Hooks issue because it
returns early when currentEpisode.value is null before its useEffect hooks run,
which changes hook order between renders. Move the guard below the hooks in
Player and keep the hooks unconditional, then add the null check inside each
useEffect body (or otherwise guard the effect logic) so the hook order stays
stable.
In `@src/lib/navLinks.ts`:
- Around line 15-20: The navLinks constant includes a show-specific hardcoded
Store URL, which makes the reusable template ship an чужд/incorrect link for
adopters. Update navLinks in navLinks.ts to remove that show-specific entry or
gate it behind configuration, and review the related sponsor link in the same
array to ensure only template-safe defaults remain.
---
Outside diff comments:
In `@src/components/FullPlayButton.tsx`:
- Around line 59-68: `FullPlayButton` updates `currentEpisode` and `isPlaying`,
but leaves `mediaMode` and `isTheaterOpen` stale, so playback can inherit the
previous CTA state. In the `FullPlayButton` click flow where
`currentEpisode.value` is assigned, explicitly set `mediaMode.value` to the
intended mode for this button and update `isTheaterOpen.value` accordingly when
starting video playback, or force audio mode if this action should remain
audio-only. Keep the behavior localized to this handler so future clicks always
start from a predictable state.
---
Nitpick comments:
In `@src/components/EpisodeCta.tsx`:
- Line 100: Simplify the conditional assignment in EpisodeCta so isPlaying.value
uses becomingCurrent directly instead of a ternary that returns true/false. Also
make the icon rendering consistent between the two branches by either removing
the unnecessary JSX.Element cast on the icon in the current branch or adding the
same cast in the pill variant branch, using the existing render logic around
EpisodeCta and its icon/pill JSX.
In `@src/lib/rss.ts`:
- Around line 101-104: The HLS alternate lookup in rss.ts is too strict because
the @'_type' comparison in the alternates.find logic only matches one exact
case/form. Update the HLS_TYPE matching in the relevant rss parsing flow to do a
case-insensitive comparison and accept common variants like
application/x-mpegurl and application/vnd.apple.mpegurl, while keeping the
existing hls selection behavior intact.
In `@src/pages/`[episode].astro:
- Around line 170-183: The EpisodeCta usage is repeating the same episode object
shape in multiple places, so hoist that literal into a single frontmatter
constant in [episode].astro and reuse it wherever EpisodeCta is rendered. Update
the repeated EpisodeCta calls to reference the shared object (using the existing
audio, video, episodeImage, episodeNumber, id, and title fields) so the field
list stays synchronized across all instances.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af3b7ae5-a36b-496e-a50d-cce525b343ab
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
.impeccable/briefs/reshape-sticky-rail.md.impeccable/design.json.impeccable/live/config.jsonDESIGN.mdPRODUCT.mdpackage.jsonsrc/components/EpisodeCta.tsxsrc/components/EpisodeList.astrosrc/components/FullPlayButton.tsxsrc/components/MobileNav.tsxsrc/components/Player.tsxsrc/components/player/ForwardButton/index.tsxsrc/components/player/PlayButton.tsxsrc/components/player/PlaybackRateButton.tsxsrc/components/player/RewindButton/index.tsxsrc/components/player/Slider/index.tsxsrc/components/state.tssrc/layouts/Layout.astrosrc/lib/cta.tssrc/lib/navLinks.tssrc/lib/rss.tssrc/pages/[episode].astrosrc/styles/global.csssrc/utils/config.tstests/unit/Player.test.tsx
| 1. **Config validation:** `defineStarpodConfig` is currently an identity function | ||
| (no Valibot), despite CLAUDE.md. **Default:** keep the identity-function pattern | ||
| for the new `cta` field; don't introduce Valibot in this pass. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Align the config-validation default with repository standards.
Line 145-147 currently recommends skipping Valibot, which conflicts with the project rule and creates contradictory implementation guidance.
As per coding guidelines, src/utils/config.ts: "Use Valibot for configuration validation in starpod.config.ts."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.impeccable/briefs/reshape-sticky-rail.md around lines 145 - 147, The
config-validation guidance is inconsistent with the repository standard: update
the `defineStarpodConfig` guidance for the new `cta` field to recommend
Valibot-based validation instead of an identity-function/default skip. Keep the
instructions aligned with the existing config pattern in `defineStarpodConfig`
and `starpod.config.ts`, and remove any wording that tells implementers not to
introduce Valibot in this pass.
Source: Coding guidelines
| - **Focus:** Border color shifts to the focused hairline tone; **ring and outline | ||
| are explicitly suppressed** (`!ring-0`, `!outline-none`) — the border shift is | ||
| the entire focus signal. Maintain a visible, AA-contrast focus indicator when |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section in DESIGN.md
git ls-files DESIGN.md
wc -l DESIGN.md
sed -n '270,305p' DESIGN.mdRepository: shipshapecode/starpod
Length of output: 2067
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section in DESIGN.md
git ls-files DESIGN.md
wc -l DESIGN.md
sed -n '270,305p' DESIGN.mdRepository: shipshapecode/starpod
Length of output: 2067
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Tailwind-related version/config information
git ls-files | rg '(^|/)(package.json|pnpm-lock.yaml|package-lock.json|yarn.lock|tailwind\\.config\\.|tailwind\\.css|postcss\\.config\\.)$'
rg -n '"tailwindcss"|tailwindcss@|outline-hidden|outline-none' package.json pnpm-lock.yaml package-lock.json yarn.lock tailwind.config.* postcss.config.* DESIGN.md 2>/dev/null || trueRepository: shipshapecode/starpod
Length of output: 839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Tailwind-related version/config information
git ls-files | rg '(^|/)(package.json|pnpm-lock.yaml|package-lock.json|yarn.lock|tailwind\\.config\\.|tailwind\\.css|postcss\\.config\\.)$'
rg -n '"tailwindcss"|tailwindcss@|outline-hidden|outline-none' package.json pnpm-lock.yaml package-lock.json yarn.lock tailwind.config.* postcss.config.* DESIGN.md 2>/dev/null || trueRepository: shipshapecode/starpod
Length of output: 839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Tailwind-related version/config information
git ls-files | rg '(^|/)(package.json|pnpm-lock.yaml|package-lock.json|yarn.lock|tailwind\\.config\\.|tailwind\\.css|postcss\\.config\\.)$'
rg -n '"tailwindcss"|tailwindcss@|outline-hidden|outline-none' package.json pnpm-lock.yaml package-lock.json yarn.lock tailwind.config.* postcss.config.* DESIGN.md 2>/dev/null || trueRepository: shipshapecode/starpod
Length of output: 839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Tailwind version/config declarations
git ls-files | rg '(^|/)(package.json|pnpm-lock.yaml|package-lock.json|yarn.lock|tailwind\\.config\\.|postcss\\.config\\.)$'
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock; do
[ -f "$f" ] && { echo "== $f =="; rg -n '"tailwindcss"|tailwindcss@|outline-hidden|outline-none' "$f" || true; }
doneRepository: shipshapecode/starpod
Length of output: 651
🌐 Web query:
Tailwind CSS v4 outline-none outline-hidden docs
💡 Result:
In Tailwind CSS v4, the outline-none and outline-hidden utilities serve distinct purposes for managing browser focus styles [1][2]. outline-none In v4, this utility now sets the CSS outline-style property to none [3][2]. This is a departure from Tailwind CSS v3, where outline-none was implemented as a 2px solid transparent outline with a 2px offset to maintain visibility for users in forced-colors modes [4][5][6]. outline-hidden This utility was introduced in v4 to provide the behavior that outline-none previously offered [3][2]. It is specifically designed to hide the default browser outline while still preserving a visible focus state for users when forced-colors mode is active [1]. Recommendation Tailwind documentation highly recommends that you apply your own custom focus styling for accessibility whenever you use either of these utilities [1]. Use outline-hidden if you need to ensure accessibility compliance for users with forced-colors settings enabled [1].
Citations:
- 1: https://tailwindcss.com/docs/outline-style
- 2: https://www.youtube.com/watch?v=ud913ekwAOQ
- 3:
outline-hiddenonly applies transparent outline whenforced-colorsis active? tailwindlabs/tailwindcss#16926 - 4: https://v3.tailwindcss.com/docs/outline-style
- 5: https://kombai.com/tailwind/outline-style/
- 6: https://tailscan.com/tailwind/borders/outline-none
Use outline-hidden for Tailwind v4 focus styling. !outline-none is the v4 utility that removes the outline style; if the intent is to hide the default outline while preserving forced-colors behavior, switch to outline-hidden instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DESIGN.md` around lines 281 - 283, The focus styling guidance currently tells
readers to use !outline-none, but for Tailwind v4 the intended utility is
outline-hidden when hiding the default outline while preserving forced-colors
behavior. Update the focus indicator guidance in DESIGN.md to reference
outline-hidden instead of !outline-none, keeping the rest of the border-focused
behavior described around the focus state intact.
| - **Identity rail:** A fixed left column (`28rem`/`lg`, `30rem`/`xl`) holding | ||
| artwork, title, blurb, listen links, and hosts. On mobile it collapses; rail | ||
| content (hosts, info) moves into the footer. | ||
| - **Nav links (`.animated-bottom-border`):** Body type with an **underline that | ||
| grows from left to right on hover** (gradient: Iris in light, Aqua in dark, | ||
| 500ms ease). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update mobile navigation docs to match shipped layout behavior.
Line 297 says rail content moves into a footer, but the current architecture uses a mobile slide-over nav and removes the mobile-only footer. This doc mismatch will mislead follow-up changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DESIGN.md` around lines 295 - 300, The mobile navigation docs are out of sync
with the shipped layout behavior: the identity rail no longer moves its content
into a footer on mobile, and the architecture uses a mobile slide-over nav
instead. Update the relevant design text around the Identity rail and nav
behavior to describe the actual mobile slide-over pattern and remove any mention
of a mobile-only footer so future changes reference the correct structure.
| if (currentEpisode.value === null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/components/Player.tsx
git log --oneline -5 -- src/components/Player.tsxRepository: shipshapecode/starpod
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of Player.tsx with line numbers.
sed -n '1,260p' src/components/Player.tsx | nl -ba | sed -n '1,260p'
# Map the hook usage structurally, if possible, to confirm ordering.
ast-grep run --pattern $'function Player() {\n $$$\n}' --lang tsx src/components/Player.tsxRepository: shipshapecode/starpod
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of Player.tsx with line numbers.
sed -n '1,260p' src/components/Player.tsx | nl -ba | sed -n '1,260p'
# Map the hook usage structurally, if possible, to confirm ordering.
ast-grep run --pattern $'function Player() {\n $$$\n}' --lang tsx src/components/Player.tsxRepository: shipshapecode/starpod
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of Player.tsx with line numbers.
sed -n '1,260p' src/components/Player.tsx | nl -ba | sed -n '1,260p'
# Map the hook usage structurally, if possible, to confirm ordering.
ast-grep run --pattern $'function Player() {\n $$$\n}' --lang tsx src/components/Player.tsxRepository: shipshapecode/starpod
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/components/Player.tsx | sed -n '1,240p'Repository: shipshapecode/starpod
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the file with line numbers without relying on `nl`.
awk '{printf("%6d %s\n", NR, $0)}' src/components/Player.tsx | sed -n '1,240p'
# Structural outline for hook ordering.
ast-grep outline src/components/Player.tsx --view expandedRepository: shipshapecode/starpod
Length of output: 9987
Rules-of-Hooks violation: move the early return below the hooks. Player returns when currentEpisode.value === null before the useEffect calls, so hook order changes between the empty and loaded states. Keep the hooks unconditional and guard the effect bodies instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Player.tsx` around lines 28 - 30, The Player component has a
Rules-of-Hooks issue because it returns early when currentEpisode.value is null
before its useEffect hooks run, which changes hook order between renders. Move
the guard below the hooks in Player and keep the hooks unconditional, then add
the null check inside each useEffect body (or otherwise guard the effect logic)
so the hook order stays stable.
| export const navLinks: Array<NavLink> = [ | ||
| { label: 'About', href: '/about' }, | ||
| { label: 'Contact', href: '/contact' }, | ||
| { label: 'Store', href: 'https://whiskey.fund/' }, | ||
| { label: 'Become a sponsor', href: '/sponsor' } | ||
| ]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Show-specific URL hardcoded in a template-level constant.
Starpod is a reusable podcast template, but { label: 'Store', href: 'https://whiskey.fund/' } (and arguably /sponsor) is specific to one show. Adopters who copy this template will ship a broken/foreign Store link. Until the planned config migration noted above lands, consider dropping the show-specific entry or moving it behind config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/navLinks.ts` around lines 15 - 20, The navLinks constant includes a
show-specific hardcoded Store URL, which makes the reusable template ship an
чужд/incorrect link for adopters. Update navLinks in navLinks.ts to remove that
show-specific entry or gate it behind configuration, and review the related
sponsor link in the same array to ensure only template-safe defaults remain.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes