Skip to content

Add detection button for multiple detections from one bout #910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benjifriedman wants to merge 1 commit into
base: main
Choose a base branch
from
+79 −4
Open

Add detection button for multiple detections from one bout #910

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 79 additions & 4 deletions ui/src/components/Bouts/BoutPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ import SpectrogramTimeline, {
SpectrogramControls,
} from "@/components/Bouts/SpectrogramTimeline";
import { BoutPlayer, PlayerControls } from "@/components/Player/BoutPlayer";
import DetectionDialog from "@/components/Player/DetectionDialog";
import type { VideoJSPlayer } from "@/components/Player/VideoJS";
import {
AudioCategory,
BoutQuery,
Expand All @@ -60,6 +62,7 @@ import {
useUpdateBoutMutation,
} from "@/graphql/generated";
import { useAudioImageUpdatedSubscription } from "@/hooks/useAudioImageUpdatedSubscription";
import useFeedPresence from "@/hooks/useFeedPresence";
import { roundToNearest } from "@/utils/time";

import CopyToClipboardButton from "../CopyToClipboard";
Expand Down Expand Up @@ -101,10 +104,15 @@ export default function BoutPage({
[],
);
const playerControls = useRef<PlayerControls>();
const setPlayerControls = useCallback(
(controls: PlayerControls) => (playerControls.current = controls),
[],
);
const [videoJsPlayer, setVideoJsPlayer] = useState<
VideoJSPlayer | undefined
>();
const [isPlaying, setIsPlaying] = useState(false);
const setPlayerControls = useCallback((controls: PlayerControls) => {
playerControls.current = controls;
setVideoJsPlayer(controls.player);
setIsPlaying(!controls.paused());
}, []);
const spectrogramControls = useRef<SpectrogramControls>();

const [cachedPlayerTime, setCachedPlayerTime] = useState<Date>(targetTime);
Expand Down Expand Up @@ -162,6 +170,26 @@ export default function BoutPage({
max: Date;
}>({ min: timelineStartTime, max: timelineEndTime });

// Track player play/pause state from the embedded Video.js player
useEffect(() => {
const player = videoJsPlayer;
if (!player) return;
const onPlaying = () => setIsPlaying(true);
const onPause = () => setIsPlaying(false);
const onWaiting = () => setIsPlaying(false);
const onError = () => setIsPlaying(false);
player.on("playing", onPlaying);
player.on("pause", onPause);
player.on("waiting", onWaiting);
player.on("error", onError);
return () => {
player.off("playing", onPlaying);
player.off("pause", onPause);
player.off("waiting", onWaiting);
player.off("error", onError);
};
}, [videoJsPlayer]);

const expandTimelineStart = useCallback(() => {
setTimelineStartTime((timelineStartTime) =>
roundToNearest(
Expand Down Expand Up @@ -214,6 +242,14 @@ export default function BoutPage({
[feedStreamQueryResult],
);
const feedStream = feedStreams[0];
const playlistTimestampNum = feedStream
? Number(feedStream.playlistTimestamp)
: undefined;
const getOffsetSeconds = () =>
feedStream && playerTime.current
? playerTime.current.valueOf() / 1000 -
Number(feedStream.playlistTimestamp)
: undefined;

const detections = detectionQueryResult.data?.detections?.results ?? [];

Expand All @@ -237,6 +273,15 @@ export default function BoutPage({
({ id }) => id,
);

// Listener count via Presence (same approach as Player.tsx)
const feedPresence = useFeedPresence(feed.slug);
const listenerCount: number = Array.isArray(
(feedPresence as Record<string, unknown[] | undefined> | undefined)?.metas,
)
? ((feedPresence as Record<string, unknown[] | undefined>)?.metas?.length ??
0)
: 0;

Comment on lines +276 to +284

@coderabbitai coderabbitai Bot Aug 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Listener count derivation is broken: wrong identifier and wrong shape assumptions

  • useFeedPresence expects a feed ID but you pass feed.slug.
  • The hook returns an object with metas (from Phoenix Presence), not a Record keyed by metas. Current casting makes listenerCount always 0.

Fix both issues.

Apply this diff:

-  const feedPresence = useFeedPresence(feed.slug);
-  const listenerCount: number = Array.isArray(
-    (feedPresence as Record<string, unknown[] | undefined> | undefined)?.metas,
-  )
-    ? ((feedPresence as Record<string, unknown[] | undefined>)?.metas?.length ??
-      0)
-    : 0;
+  const feedPresence = useFeedPresence(feed.id) as { metas?: unknown[] } | undefined;
+  const listenerCount: number = Array.isArray(feedPresence?.metas)
+    ? feedPresence!.metas!.length
+    : 0;

If you'd like, I can also open a follow-up to correct the return type of useFeedPresence so consumers don’t need casts.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Listener count via Presence (same approach as Player.tsx)
const feedPresence = useFeedPresence(feed.slug);
const listenerCount: number = Array.isArray(
(feedPresence as Record<string, unknown[] | undefined> | undefined)?.metas,
)
? ((feedPresence as Record<string, unknown[] | undefined>)?.metas?.length ??
0)
: 0;
// Listener count via Presence (same approach as Player.tsx)
- const feedPresence = useFeedPresence(feed.slug);
- const listenerCount: number = Array.isArray(
- (feedPresence as Record<string, unknown[] | undefined> | undefined)?.metas,
- )
- ? ((feedPresence as Record<string, unknown[] | undefined>)?.metas?.length ??
- 0)
const feedPresence = useFeedPresence(feed.id) as { metas?: unknown[] } | undefined;
const listenerCount: number = Array.isArray(feedPresence?.metas)
? feedPresence!.metas!.length
: 0;
🤖 Prompt for AI Agents 
In ui/src/components/Bouts/BoutPage.tsx around lines 276 to 284, the listener
count is computed incorrectly: pass feed.slug (should be feed.id) to
useFeedPresence and the code incorrectly assumes the hook returns a Record keyed
by metas, causing listenerCount to always be 0. Change the hook call to
useFeedPresence(feed.id), treat the return value as the Presence object (with an
optional metas array), and compute listenerCount as the length of presence.metas
if present (e.g., const listenerCount = Array.isArray(presence?.metas) ?
presence.metas.length : 0), removing the incorrect Record casts.

const [boutForm, setBoutForm] = useState<{
errors: Record<string, string>;
isSaving: boolean;
Expand Down Expand Up @@ -604,6 +649,36 @@ export default function BoutPage({
</Box>
)}
</Box>
{feedStream && (
<Box
display="flex"
flexDirection="column"
alignItems="center"
minWidth={120}
>
<Box>
<Typography variant="overline">Add detection</Typography>
</Box>
<Box>
<DetectionDialog
feed={feed}
timestamp={playlistTimestampNum}
isPlaying={isPlaying}
getPlayerTime={getOffsetSeconds}
listenerCount={listenerCount}
>
Comment on lines +663 to +669

@coderabbitai coderabbitai Bot Aug 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Prop uses the old timestamp variable; align with seconds-based value

If you adopt the seconds-based playlistTimestampSec above, wire it through here to keep DetectionDialog submissions consistent.

Apply this diff:

-                <DetectionDialog
+                <DetectionDialog
                   feed={feed}
-                  timestamp={playlistTimestampNum}
+                  timestamp={playlistTimestampSec}
                   isPlaying={isPlaying}
                   getPlayerTime={getOffsetSeconds}
                   listenerCount={listenerCount}
                 >
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<DetectionDialog
feed={feed}
timestamp={playlistTimestampNum}
isPlaying={isPlaying}
getPlayerTime={getOffsetSeconds}
listenerCount={listenerCount}
>
<DetectionDialog
feed={feed}
timestamp={playlistTimestampSec}
isPlaying={isPlaying}
getPlayerTime={getOffsetSeconds}
listenerCount={listenerCount}
>
🤖 Prompt for AI Agents 
In ui/src/components/Bouts/BoutPage.tsx around lines 663 to 669, the
DetectionDialog prop 'timestamp' is still using the old seconds-based variable
playlistTimestampNum; replace it with the seconds-based variable
playlistTimestampSec so DetectionDialog receives the correct unit. Update the
prop to timestamp={playlistTimestampSec}, confirm the variable is defined in
scope and typed as seconds (number), and run type-checks/tests to ensure no
other callsites rely on the old variable name/unit.

<IconButton
title="Add detection"
color="secondary"
size="small"
aria-label="Add detection"
>
<GraphicEq />
</IconButton>
</DetectionDialog>
</Box>
</Box>
)}
<Box
display="flex"
flexDirection="column"
Expand Down
Loading