Refactor video detail component architecture

1. TL;DR

• Rename VideoContentTabs to VideoCompanionSidebar and keep it conceptually “the right companion column” (Chat + Chapters).
• Extract a small FixedSidebarLayout (or VideoDetailLayout) shell and 1–2 micro-components from VideoDetail so that file reads like a clear story instead of a wall of JSX and glue code.
• Remove derived state / unused hooks, and centralize playlist navigation + “is video out of view” logic into small hooks to make the page logic obvious at a glance.

Effort: M (1–3 hours) for the core refactor, more if you decide to aggressively delete legacy props and APIs.

1. Naming: VideoContentTabs → VideoCompanionSidebar

Your right sidebar is not “tabs” in the app’s mental model; it’s “the companion panel for this video (chat + chapters)”. I recommend:

New name: VideoCompanionSidebar

• Component: VideoCompanionSidebar
• File: video-companion-sidebar.tsx
• Props interface: VideoCompanionSidebarProps
• Type SidebarTab can remain as-is or be renamed to CompanionTab.

Rationale:

• Aligns with PlaylistSidebar naming and the actual role: it’s the sidebar that acts as a companion to the video (assistant + chapter navigation).

• Decouples from implementation detail (“tabs”) that may change.

• Reads well in the page:

  <VideoCompanionSidebar ... />
  <PlaylistSidebar ... />
  

That “sings” more than VideoContentTabs.

2. Simplifying VideoDetail: Extract the layout and a couple of subcomponents

2.1 Extract the fixed 3-column shell

Right now VideoDetail is doing:

• Layout shell concerns (positioning left/right sidebars, padding for center).
• Page logic (start time, autoplay, scroll, playlist nav).
• Main content rendering (player, metadata, tabs, CTA, transcript).

The layout shell noise obscures the interesting logic. Extract to a reusable component; this can live in components/layout/fixed-sidebar-layout.tsx or next to the page as video-detail-layout.tsx.

Example:

// components/layout/fixed-sidebar-layout.tsx
"use client";

import { ReactNode } from "react";
import { cn } from "@/lib/utils";

interface FixedSidebarLayoutProps {
  leftSidebar?: ReactNode;
  rightSidebar?: ReactNode;
  children: ReactNode;
  className?: string; // for the inner max-width container if needed
}

export function FixedSidebarLayout({
  leftSidebar,
  rightSidebar,
  children,
  className,
}: FixedSidebarLayoutProps) {
  const hasLeft = !!leftSidebar;
  const hasRight = !!rightSidebar;

  return (
    <div className="min-h-full relative">
      {leftSidebar}
      <div
        className="flex-1 min-w-0 w-full pt-5"
        style={{
          paddingLeft: hasLeft
            ? "calc(var(--sidebar-left-width) + 20px)"
            : "20px",
          paddingRight: hasRight
            ? "calc(var(--sidebar-right-width) + 20px)"
            : "20px",
        }}
      >
        <div className={cn("mx-auto", className)}>{children}</div>
      </div>
      {rightSidebar}
    </div>
  );
}

Then VideoDetail becomes structurally very clear:

return (
  <AIAssistantProvider onTimeClick={handleTimeSelect}>
    <FixedSidebarLayout
      leftSidebar={
        playlist ? (
          <PlaylistSidebar
            playlist={playlist}
            currentVideoId={video.id}
            className="z-30"
            mode="collapse"
          />
        ) : undefined
      }
      rightSidebar={
        <VideoCompanionSidebar
          videoId={video.id}
          playbackId={video.playback_id}
          chaptersWebVTT={video.chapters || ""}
          aiName={settings?.ai_name || "AI Assistant"}
          aiAvatar={settings?.ai_avatar || "/default-avatar.png"}
          subdomain=""
          greeting={settings?.ai_greeting}
          onChapterClick={handleTimeSelect}
          hasChapters={hasChapters}
          className="z-[40]"
        />
      }
      className="max-w-4xl"
    >
      {/* Main column content goes here (player, metadata, description/transcript) */}
    </FixedSidebarLayout>
  </AIAssistantProvider>
);

Note: we pushed the className="max-w-4xl mx-auto" into the layout; now VideoDetail doesn’t worry about padding or positioning – just content and behavior.

2.2 Extract the main content body into a VideoMainContent subcomponent

From VideoDetail, extract everything under the player’s “content below player” into a child. That keeps VideoDetail focused on wiring + orchestration.

New component skeleton:

// components/video-main-content.tsx
"use client";

import { useState } from "react";
import Link from "next/link";
import ReactMarkdown from "react-markdown";
import remarkGfm from "remark-gfm";
import { ChevronLeft, ChevronRight } from "lucide-react";
import { cn } from "@/lib/utils";
import { Transcript } from "@/components/transcript";
import { AutoplayToggle } from "./autoplay-toggle";
import { VideoDescription } from "./video-description";
import type { ExtendedVideo } from "./video-detail"; // or extract this type to a shared file
import type { Playlist } from "@boldvideo/bold-js";

interface VideoMainContentProps {
  video: ExtendedVideo;
  playlist?: Playlist;
  hasTranscript: boolean;
  currentVideoIndex: number;
  previousVideo?: Playlist["videos"][number] | null;
  nextVideo?: Playlist["videos"][number] | null;
  hasPreviousVideo: boolean;
  hasNextVideo: boolean;
  onTimeSelect: (time: number) => void;
  playerRef: React.RefObject<HTMLVideoElement>;
}

export function VideoMainContent({
  video,
  playlist,
  hasTranscript,
  currentVideoIndex,
  previousVideo,
  nextVideo,
  hasPreviousVideo,
  hasNextVideo,
  onTimeSelect,
  playerRef,
}: VideoMainContentProps) {
  const [activeMainTab, setActiveMainTab] =
    useState<"description" | "transcript">("description");

  // (mobile controls + title + metadata + main tabs + CTA + Transcript)
  // ... move your existing JSX here, just wired through props.
}

Then in VideoDetail you just compute the playlist navigation info and pass it down.

This alone makes video-detail.tsx read like:

• setup hooks
• derive navigation
• handlers (time select, video ended)
• return (AIAssistantProvider + FixedSidebarLayout + Player + VideoMainContent)

instead of also rendering 300+ lines of UI.

3. Hooks / logic to extract or simplify

3.1 Derived values should stay derived, not in state

In VideoDetail:

const [video] = useState<ExtendedVideo>(initialVideo);
const [hasTranscript, setHasTranscript] = useState(false);

useEffect(() => {
  setHasTranscript(
    !!video.transcript &&
      !!video.transcript.json &&
      !!video.transcript.json.url
  );
}, [video]);

You never change video. And hasTranscript is a pure derivation from video.

Refactor:

const video = initialVideo;
const hasTranscript = Boolean(video.transcript?.json?.url);

Result: fewer hooks, fewer moving parts; intent is obvious.

3.2 Extract playlist navigation into a small hook

Right now:

const currentVideoIndex =
  playlist?.videos.findIndex((v) => v.id === video.id) ?? -1;
const hasPreviousVideo = playlist && currentVideoIndex > 0;
const hasNextVideo = playlist &&
  currentVideoIndex >= 0 &&
  currentVideoIndex < playlist.videos.length - 1;

const previousVideo = hasPreviousVideo
  ? playlist.videos[currentVideoIndex - 1]
  : null;
const nextVideo = hasNextVideo
  ? playlist.videos[currentVideoIndex + 1]
  : null;

That’s good logic, but it clutters VideoDetail.

Hook:

// hooks/use-playlist-navigation.ts
import type { Playlist } from "@boldvideo/bold-js";

export function usePlaylistNavigation(playlist: Playlist | undefined, videoId: string) {
  if (!playlist) {
    return {
      currentIndex: -1,
      hasPrevious: false,
      hasNext: false,
      previous: null,
      next: null,
    };
  }

  const currentIndex = playlist.videos.findIndex((v) => v.id === videoId);
  const hasPrevious = currentIndex > 0;
  const hasNext = currentIndex >= 0 && currentIndex < playlist.videos.length - 1;

  return {
    currentIndex,
    hasPrevious,
    hasNext,
    previous: hasPrevious ? playlist.videos[currentIndex - 1] : null,
    next: hasNext ? playlist.videos[currentIndex + 1] : null,
  };
}

Use it in VideoDetail:

const {
  currentIndex: currentVideoIndex,
  hasPrevious: hasPreviousVideo,
  hasNext: hasNextVideo,
  previous: previousVideo,
  next: nextVideo,
} = usePlaylistNavigation(playlist, video.id);

This makes handleVideoEnded and the mobile controls read much cleaner.

3.3 Extract “scroll visibility” into a hook (and fix the logic)

Current code:

const [isOutOfView, setIsOutOfView] = useState<boolean>(false);
const prevScrollY = useRef(0);

const handleScroll = useCallback(() => {
  const currentScrollY = window.scrollY;
  if (currentScrollY > window.innerHeight * 0.7 && !isOutOfView) {
    setIsOutOfView(false);
  }
  if (currentScrollY < window.innerHeight * 0.7 && isOutOfView) {
    setIsOutOfView(false);
  }
  prevScrollY.current = currentScrollY;
}, [isOutOfView]);

This currently never sets true and the two branches call setIsOutOfView(false) in both cases – clearly a smell/bug.

Hook:

// hooks/use-scroll-out-of-view.ts
import { useEffect, useState } from "react";

export function useScrollOutOfView(thresholdRatio: number = 0.7) {
  const [isOutOfView, setIsOutOfView] = useState(false);

  useEffect(() => {
    const handleScroll = () => {
      const currentScrollY = window.scrollY;
      const threshold = window.innerHeight * thresholdRatio;
      setIsOutOfView(currentScrollY > threshold);
    };

    window.addEventListener("scroll", handleScroll, { passive: true });
    handleScroll(); // initialize
    return () => window.removeEventListener("scroll", handleScroll);
  }, [thresholdRatio]);

  return isOutOfView;
}

Then in VideoDetail:

const isOutOfView = useScrollOutOfView(0.7);

Now the Player prop is a simple, correct boolean, and the scroll behavior is encapsulated and testable.

3.4 Unify start time responsibilities

You currently:

• Pass startTime={effectiveStartTime} to <Player>.
• Also directly manipulate playerRef.current.currentTime in an effect:

useEffect(() => {
  if (effectiveStartTime && playerRef.current) {
    playerRef.current.currentTime = effectiveStartTime;
  }
}, [effectiveStartTime]);

Prefer one approach for clarity.

Recommendation: Let Player own seeking by startTime, and remove the manual effect unless there is a specific known bug with Player.

If you must keep the effect, at least document why in a short comment so future readers aren’t confused.

3.5 Remove unused useSidebar destructuring

At the top of VideoDetail:

const { left, right, isMobile } = useSidebar();

Those variables are not used at all in this component; layout already uses CSS vars set by SidebarProvider. Delete this line to reduce cognitive load.

4. Patterns that smell (and suggested cleanup)

1. “Legacy” props in VideoContentTabs that aren’t honored.

   activeTab?: VideoTabId;
   onTabChange?: (tab: VideoTabId) => void;
   hasTranscript?: boolean;
   showAI?: boolean;
   

   But the component implements its own state + localStorage and never calls onTabChange. That’s misleading.

   • Either:
     • Implement full controlled/uncontrolled behavior (respect activeTab, call onTabChange), or
     • Remove these props (or mark them clearly as deprecated in JSDoc and never use them).

   For “code that sings,” I’d pick one: keep it simple and uncontrolled (activeTab internal only), and delete the dead props.

2. Duplicated concerns around sidebars:

   • PlaylistSidebar and VideoContentTabs both contain small hacks to “ensure sidebar is open in context for desktop when in collapse mode”.
   • This is fine, but consider extracting a tiny helper like useEnsureCollapsedSidebarOpen("left" | "right") or adding a mode="collapse"-aware behavior to Sidebar itself to reduce duplication.

3. Derived UI state persisted in different ways:

   • Right sidebar tab (VideoContentTabs) uses localStorage to persist.
   • Main content tab (description/transcript) does not.

   That’s okay, but be intentional. A short comment on why one persists and the other doesn’t would help future maintainers.

4. CTA block inline in VideoDetail.

   The CTA rendering is nicely self-contained; it’d be even cleaner as:

   <VideoCTA cta={video.cta} />
   

   That reduces “vertical noise” in VideoMainContent and helps the file read in higher-level terms: Description → CTA → Transcript.

5. Recommended refactor plan (simple path)

1. Rename the right sidebar component:

   • Rename file video-content-tabs.tsx → video-companion-sidebar.tsx.
   • Change component export to VideoCompanionSidebar.
   • Update imports in video-detail.tsx and anywhere else.

2. Introduce FixedSidebarLayout:

   • Create components/layout/fixed-sidebar-layout.tsx (as above).
   • Replace the outer <div className="min-h-full relative"> + padded main area + bottom <VideoContentTabs /> in VideoDetail with this layout.

3. Extract VideoMainContent:

   • Move everything under the player (mobile playlist controls, title, metadata, description/transcript tabs, CTA, transcript) into a new VideoMainContent component.
   • Keep only high-level wiring in VideoDetail.

4. Add usePlaylistNavigation hook:

   • Create hooks/use-playlist-navigation.ts (as shown).
   • Replace inlined prev/next logic in VideoDetail.

5. Fix derived state / scroll logic:

   • Inline hasTranscript as a boolean expression.
   • Remove useState for video, and just do const video = initialVideo.
   • Remove unused useSidebar destructuring.
   • Create useScrollOutOfView and replace current scroll handler with it.
   • Decide if Player should be solely responsible for seeking; if yes, remove the manual currentTime effect.

6. Clean up legacy props in VideoCompanionSidebar:

   • Remove or clearly deprecate activeTab, onTabChange, hasTranscript, showAI from the props interface if they are not used.

This path keeps your existing concepts and dependencies, but makes each file much more readable and narration-like.

6. Rationale and trade-offs

• Why this is enough now:
  • Your current architecture is fundamentally sound: a fixed 3-column app shell with stateful sidebars.
  • The pain today is readability, not capability. Extracting layout and a couple of hooks/components directly addresses the “code singing” goal without over-engineering.
• Why not a deeper layout system?
  • You could build a generic “AppShell” for the whole app, but that’s a bigger design decision and not required just to make this specific page clear.
  • YAGNI: refactor this page cleanly first; factor out to a larger shared layout only if multiple pages start to need the same pattern.

7. Risks and guardrails

• Risk: subtle CSS / spacing regressions when extracting FixedSidebarLayout.
  • Guardrail: keep the exact padding calculations and classes; write a visual regression note (or snapshot) for desktop + mobile with/without playlist.
• Risk: behavior around isOutOfView changes.
  • Guardrail: confirm what the Player does with isOutOfView (e.g., mini-player behavior). Test scroll behavior manually after moving to useScrollOutOfView.
• Risk: localStorage / sessionStorage interactions in sidebars.
  • Guardrail: test a) first load, b) reload, c) resize across mobile/desktop, focusing on right sidebar tab persistence and open/closed state.

8. When to consider the advanced path

Revisit the design and consider a more global architecture if:

• You add more pages with the same “fixed 3-column” pattern, or
• The right sidebar needs more modes (e.g., “Resources”, “Notes”), or
• You start supporting embedded players in different shells (docs, blog, etc.), where sidebars become composable “slots.”

Then it may be worth:

• A full AppShell (header + sidebars + main).
• A more generic CompanionPanel system with pluggable sections.

9. Optional advanced path (brief)

If/when you go further:

• Create AppShell:

  <AppShell
    leftSidebar={<PlaylistSidebar ... />}
    rightSidebar={<VideoCompanionSidebar ... />}
  >
    <VideoMainContent ... />
  </AppShell>
  

• Define “page sections” (VideoHeader, VideoPlayerSection, VideoDetailsSection) as composable blocks.

• Standardize tab systems (use a single Tabs primitive) and side panel behaviors across pages.

For now, though, the simple refactor (rename + layout extraction + a couple of hooks) will already make this codebase feel significantly cleaner and more musical.