Add detection button for multiple detections from one bout#910
Add detection button for multiple detections from one bout#910benjifriedman wants to merge 1 commit intomainfrom
Conversation
WalkthroughIntegrated Video.js player state and event syncing, computed playback offset from playlist timestamp, derived listener presence count, and wired a new DetectionDialog for adding detections using feed, timing, and player state. Added two conditional “Add detection” UI blocks when a feed stream exists. Updated imports and hooks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant BoutPage
participant VideoJS as Video.js Player
participant Presence as FeedPresence
participant Dialog as DetectionDialog
Note over BoutPage: On mount or player ready
BoutPage->>VideoJS: store player ref via setPlayerControls
VideoJS-->>BoutPage: events (playing/pause/waiting/error)
BoutPage->>BoutPage: update isPlaying from player.paused()
Note over BoutPage: Feed stream available
BoutPage->>BoutPage: compute playlistTimestampNum
BoutPage->>BoutPage: getOffsetSeconds() from player currentTime + playlist timestamp
BoutPage->>Presence: useFeedPresence(feedId)
Presence-->>BoutPage: metas.length -> listenerCount
User->>BoutPage: Click "Add detection"
BoutPage->>Dialog: Open with { feed, playlistTimestampNum, isPlaying, getPlayerTime=getOffsetSeconds, listenerCount }
Dialog-->>BoutPage: Submit/close (detection added or canceled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ui/src/components/Bouts/BoutPage.tsx (2)
173-191: Prefer 'play' event over 'playing' for Video.js; minor robustness tweakVideo.js reliably emits 'play' and 'pause'; 'playing' may not consistently fire across sources/tech. Also, minor cleanups: use 'onPlay' name for parity and guard against already-disposed players (rare, but cheap).
Apply this diff:
- const onPlaying = () => setIsPlaying(true); + const onPlay = () => setIsPlaying(true); const onPause = () => setIsPlaying(false); const onWaiting = () => setIsPlaying(false); const onError = () => setIsPlaying(false); - player.on("playing", onPlaying); + player.on("play", onPlay); player.on("pause", onPause); player.on("waiting", onWaiting); player.on("error", onError); return () => { - player.off("playing", onPlaying); + player.off("play", onPlay); player.off("pause", onPause); player.off("waiting", onWaiting); player.off("error", onError); };
652-681: UX consideration: gating submission by isPlaying may block valid reportsDetectionDialog requires isPlaying to be true to submit. That could prevent users from pausing to precisely locate and then report. If that’s intentional, ignore; otherwise consider allowing submission when paused (and just use the current playhead time).
Would you like a quick change to allow submission when paused but still capture the current offset accurately?
📜 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)
ui/src/components/Bouts/BoutPage.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ui/src/components/Bouts/BoutPage.tsx (4)
ui/src/components/Player/VideoJS.tsx (1)
VideoJSPlayer(10-10)ui/src/components/Player/BoutPlayer.tsx (1)
PlayerControls(18-24)ui/src/hooks/useFeedPresence.ts (1)
useFeedPresence(11-38)ui/src/components/Player/DetectionDialog.tsx (1)
DetectionDialog(28-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
ui/src/components/Bouts/BoutPage.tsx (3)
50-52: Good addition: lazy type-only import for player type keeps bundle leanUsing a type-only import for VideoJSPlayer avoids bloating the client bundle. Importing DetectionDialog here is appropriate.
107-115: Storing player ref and deriving initial play state: LGTMPersisting the Video.js player instance from PlayerControls and initializing isPlaying from controls.paused() is correct and keeps state in sync from the outset.
245-253: No unit mismatch—playlistTimestampis in seconds.The GraphQL schema defines
playlistTimestampas anInt(UTC Unix epoch in seconds), andplayerTime.current.valueOf() / 1000converts the Date’s millisecond value to seconds. Subtracting two second‐based values yields the correct offset in seconds. You can safely ignore the suggested diff.Likely an incorrect or invalid review comment.
| // 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; | ||
|
|
There was a problem hiding this comment.
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.
| // 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.
| <DetectionDialog | ||
| feed={feed} | ||
| timestamp={playlistTimestampNum} | ||
| isPlaying={isPlaying} | ||
| getPlayerTime={getOffsetSeconds} | ||
| listenerCount={listenerCount} | ||
| > |
There was a problem hiding this comment.
🛠️ 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.
| <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.
skanderm
left a comment
skanderm
left a comment
There was a problem hiding this comment.
Thanks for this submission Benji! Looking through it, I'm realizing I should have been more specific about the context of what the feature needed. There's also a way that the DetectionDialog made some assumptions that don't apply fully here (i.e. that someone wants to make a submission while the audio is playing live), so we might actually want to create a new component for it.
Could you try this again with these notes?
- Create a new
BoutDetectionSubmissioncomponent - Have it take the params necessary to calculate the right
playerOffsetfor thesubmitDetectionmutation:playerTime(Date ref updated by the BoutPlayer),feedStream(to getplaylistTimestamp). You can later calculateplayerOffsetby usingsubMilliseconds(playerTime.current, playlistTimestamp) / 1000. (TheplayerOffsetis the time in seconds between the playlistTimestamp and the time of the detection. The playlistTimestamp is when the hydrophone node reboots every 6 or 24 hours and a new S3 bucket dir is created to upload 10s audio segments, so the offset tells us where in the new S3 dir is the detection). - Include a button that opens the dialog modal to let the user select the category and type in a description
- Submit button in the modal should send the
submitDetectionsubmission with theplaylistTimestampfrom thefeedStream, thefeedIdpassed in also as a param toBoutDetectionSubmission, theplayerOffsetcalculated earlier, alistenerCountset to 1 (because it's just the moderator listening/submitting the detection, and acategoryanddescriptionset during the modal.
All that should allow you to skip needing the VideoJSPlayer and the useEffect to set all the player hooks. Setting listenerCount to 1 also lets you skip the feedPresence stuff which was intended to give moderators a signal about how many people are listening live when they're submitting a detection, which doesn't apply here.
Please let me know if you have any questions, or if your dev setup is still missing stuff to make it easier to build this!
|
Thanks for the feedback @skanderm. I will make these changes in the pull request |
3 participants
Summary by CodeRabbit
New Features
Improvements