New bout page #730
New bout page #730
Conversation
…stream/segments populate function
WalkthroughThe pull request introduces a variety of changes ranging from configuration updates, new function additions, dependency modifications, and schema migrations, to multiple new and updated React UI components. Back-end modules are updated to improve performance, error handling, and rate limiting—especially in audio image processing and GraphQL interactions—while front-end changes add new visual components and pages. The PR also refactors gettext integration and updates dependency declarations and code generation settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Oban as Oban Job
participant Worker as GenerateSpectrogram Worker
participant RateLimiter as RateLimiter
participant DB as AudioImage Update
Oban->>Worker: Pass job with audio_image_id & attempt
Worker->>RateLimiter: Check rate limit (continue?/3)
RateLimiter-->>Worker: :ok / denies then sleep and retry
Worker->>DB: Process and update audio image
alt Successful Processing
DB-->>Worker: Return :ok
else Error & Attempt>=3
DB-->>Worker: Update audio image to failed
end
Worker-->>Oban: Return final status
sequenceDiagram
participant Caller as Application Caller
participant Client as GraphQL Client
participant Finch as Finch HTTP Client
participant API as GraphQL API
Caller->>Client: get_feed(feed_slug)
Client->>Client: Build GraphQL query
Client->>Finch: submit(query)
Finch->>API: Send HTTP POST request
API-->>Finch: Respond with JSON data
Finch-->>Client: Return response
Client-->>Caller: Return decoded result
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
… timeline styling
… be slightly more RESTful (allows bouts/[id])
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (21)
server/lib/orcasite/repo.ex (1)
23-25: Add documentation explaining the PostgreSQL version requirement.Please add
@doccomments explaining why PostgreSQL 14.0.0 is required as the minimum version. This helps other developers understand the version constraints.+ @doc """ + Returns the minimum required PostgreSQL version. + + PostgreSQL 14.0.0 is required because... + """ def min_pg_version() do %Version{major: 14, minor: 0, patch: 0} endui/src/utils/time.ts (1)
16-35: Consider adding input validation and improving type safety.The implementation is correct and well-documented. However, consider these improvements:
- Add input validation for
timeUnitMsto ensure it's positive.- Use TypeScript's literal union type for better type safety.
export const roundToNearest = ( date: Date, timeUnitMs: number, - roundMethod: "round" | "ceil" | "floor", + roundMethod: RoundMethod, ) => { + if (timeUnitMs <= 0) { + throw new Error("timeUnitMs must be positive"); + } + let roundFn; if (roundMethod === "round") { roundFn = Math.round; } else if (roundMethod === "ceil") { roundFn = Math.ceil; } else { roundFn = Math.floor; } return new Date(roundFn(date.getTime() / timeUnitMs) * timeUnitMs); }; + +type RoundMethod = "round" | "ceil" | "floor";server/lib/orcasite/radio/feed_stream.ex (1)
291-324: LGTM! Consider adding tests.The new
populate_with_segmentsaction provides a clean way to upsert feed streams with their segments. The implementation is well-structured and handles all necessary attributes and relationships.Would you like me to generate test cases for the new
populate_with_segmentsaction to ensure it handles various scenarios correctly?ui/src/pages/bouts/new/[feedSlug].tsx (1)
18-21: Consider moving audioCategories to a constants file.The audioCategories array could be moved to a shared constants file for reusability.
- const audioCategories: AudioCategory[] = useMemo( - () => ["ANTHROPHONY", "BIOPHONY", "GEOPHONY"], - [], - ); +import { AUDIO_CATEGORIES } from '@/constants/audioCategories'; + const audioCategories = useMemo(() => AUDIO_CATEGORIES, []);ui/src/pages/bouts/[boutId].tsx (1)
29-39: Consider adding metadata for SEO.While the page title is set, consider adding more metadata for better SEO.
Add more meta tags within the Head component:
<Head> <title>Bout | Orcasound</title> + <meta name="description" content="View bout details and recordings" /> + <meta property="og:title" content="Bout | Orcasound" /> + <meta property="og:description" content="View bout details and recordings" /> </Head>server/priv/repo/migrations/20250127201038_install_ash-uuid_v2_extension.exs (2)
1-7: Consider enhancing module documentation.While the documentation mentions this was auto-generated, it would be helpful to add information about the UUID v7 format and its benefits over other UUID versions.
@moduledoc """ Installs any extensions that are mentioned in the repo's `installed_extensions/0` callback This file was autogenerated with `mix ash_postgres.generate_migrations` + + UUID v7 provides time-ordered UUIDs that are: + - Sortable by creation time + - Compatible with UUID v4 random format + - Microsecond precision timestamps """
49-53: Consider implementing a safer rollback strategy.The current rollback strategy is commented out. Consider implementing a version that first checks if the functions are in use before dropping them.
def down do - # Uncomment this if you actually want to uninstall the extensions - # when this migration is rolled back: - # execute("DROP FUNCTION IF EXISTS uuid_generate_v7()") + execute(""" + DO $$ + BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_depend d + JOIN pg_proc p ON p.oid = d.objid + WHERE p.proname = 'uuid_generate_v7' + AND d.deptype = 'n' + ) THEN + DROP FUNCTION IF EXISTS uuid_generate_v7(); + DROP FUNCTION IF EXISTS timestamp_from_uuid_v7(uuid); + END IF; + END + $$; + """) endui/src/components/Bouts/TimelineMarker.tsx (2)
6-22: Consider adding accessibility props and documentation.The component could benefit from additional accessibility props and JSDoc documentation.
+/** + * A component that renders a marker on a timeline with an icon and vertical line. + * @param time - The timestamp to display + * @param timelineStartTime - The start time of the timeline + * @param pixelsPerMinute - The number of pixels per minute for scaling + * @param zIndex - The z-index for layering + * @param Icon - The icon component to display + * @param iconProps - Additional props for the icon + * @param onClick - Click handler for the marker + */ export function TimelineMarker({ time, timelineStartTime, pixelsPerMinute, zIndex, Icon, iconProps, onClick, + ariaLabel, }: { time: Date; timelineStartTime: Date; pixelsPerMinute: number; zIndex: number; Icon: SvgIconComponent; iconProps?: { [key: string]: string }; onClick?: () => void; + ariaLabel?: string; })
23-56: Add memoization for performance optimization.Consider memoizing the component to prevent unnecessary re-renders when parent components update.
+import { memo } from "react"; -export function TimelineMarker({ +export const TimelineMarker = memo(function TimelineMarker({ // ... props -}) { +}) { const offset = timeToOffset(time, timelineStartTime, pixelsPerMinute); return ( <Box height={1} title={time.toLocaleTimeString(undefined, { timeZoneName: "short" })} + role="button" + aria-label={ariaLabel ?? time.toLocaleTimeString()} sx={{ position: "absolute", left: `${offset}px`, zIndex: zIndex, cursor: "pointer", }} onClick={onClick} > // ... rest of the component ); -} +});ui/src/components/Bouts/BaseAudioWidthLayer.tsx (1)
22-41: Optimize rendering performance for large datasets.The current implementation might cause performance issues with a large number of tiles. Consider using virtualization.
+import { FixedSizeList as List } from 'react-window'; +const TileItem = memo(({ index, style, data: { pixelsPerTile, zIndex, TICKER_HEIGHT } }) => ( + <Box + key={index} + zIndex={zIndex} + bgcolor={"#bbb"} + display="flex" + alignItems="center" + justifyContent="center" + borderRight="1px solid #999" + sx={{ + ...style, + minWidth: pixelsPerTile, + minHeight: `calc(100% - ${TICKER_HEIGHT}px)`, + top: TICKER_HEIGHT, + }} + /> +)); - {Array(tiles) - .fill(0) - .map((_, idx) => ( - <Box - key={idx} - zIndex={zIndex} - bgcolor={"#bbb"} - display="flex" - alignItems="center" - justifyContent="center" - borderRight="1px solid #999" - sx={{ - minWidth: pixelsPerTile, - minHeight: `calc(100% - ${TICKER_HEIGHT}px)`, - position: "absolute", - left: idx * pixelsPerTile, - top: TICKER_HEIGHT, - }} - ></Box> - ))} + <List + height={window.innerHeight} + itemCount={tiles} + itemSize={pixelsPerTile} + width={tiles * pixelsPerTile} + itemData={{ pixelsPerTile, zIndex, TICKER_HEIGHT }} + > + {TileItem} + </List>server/lib/orcasite/radio/workers/generate_spectrogram.ex (1)
12-39: Robust job processing flow.The usage of
attemptto handle retries up to 3 times is solid. Setting the resource to:set_failedon the final retry ensures a definitive status. The rate limit usage also helps mitigate resource overuse. If additional logging or alerting is desired on repeated failures, consider extending the final retry flow.ui/src/components/Bouts/BoutPage.tsx (1)
70-579: Extensive bout management logic looks good.The integration of feed queries, form state, and timeline visualization is cohesive. For maintainability, consider extracting some form or timeline logic into a custom hook or smaller components.
ui/src/components/Player/BoutPlayer.tsx (2)
52-52: Consider memoizing thenowvalue andplayerRef.The
nowvalue andplayerRefare recreated on every render. Consider usinguseMemofornowanduseRefwith an initial value for better performance.- const now = useMemo(() => new Date(), []); - const playerRef = useRef<VideoJSPlayer | null>(null); + const now = useMemo(() => new Date(), []); + const playerRef = useRef<VideoJSPlayer | null>(null);Also applies to: 53-53
128-128: Use optional chaining for better readability.The static analysis tool suggests using optional chaining for better readability.
- setPlayerTimeRef && setPlayerTimeRef(time); + setPlayerTimeRef?.setPlayerTimeRef(time); - {playerDateTime !== undefined && - playerDateTime.toLocaleTimeString()} + {playerDateTime?.toLocaleTimeString()} - {playerDateTime !== undefined && - playerDateTime.toLocaleDateString()} + {playerDateTime?.toLocaleDateString()}Also applies to: 232-232, 236-236
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/Bouts/FeedSegmentsLayer.tsx (1)
38-44: Simplify null/undefined checks.Multiple null/undefined checks can be simplified using optional chaining and nullish coalescing.
- if ( - feedSegment.startTime !== undefined && - feedSegment.startTime !== null && - feedSegment.endTime !== undefined && - feedSegment.endTime !== null && - typeof feedSegment.duration === "string" - ) { + if (feedSegment.startTime && feedSegment.endTime && typeof feedSegment.duration === "string") {ui/src/components/Bouts/TimelineTickerLayer.tsx (2)
31-32: Extract magic numbers as named constants.The values
20and8for ticks and labels per window should be named constants for better maintainability.+const TICKS_PER_WINDOW = 20; +const LABELS_PER_WINDOW = 8; - const ticksPerWindow = 20; - const labelsPerWindow = 8; + const ticksPerWindow = TICKS_PER_WINDOW; + const labelsPerWindow = LABELS_PER_WINDOW;
36-67: Consider extracting scale arrays to a separate configuration file.The tick and label scales are duplicated and could be maintained in a separate configuration file.
Create a new file
timelineScales.ts:export const TIMELINE_SCALES = [ hoursToMilliseconds(24), hoursToMilliseconds(6), // ... rest of the scales ];ui/src/components/Bouts/FrequencyAxisLayer.tsx (1)
25-45: Optimize tick calculation performance.The tick calculation involves multiple array operations that could be optimized.
Consider memoizing the ticks calculation:
+ const ticks = useMemo(() => { return _.range(minScale, maxScale).flatMap((scale) => { // ... existing calculation }); + }, [minScale, maxScale, adjustedMinFrequency, maxFrequency]);server/lib/orcasite/radio/graphql_client.ex (2)
14-71: Extract or modularize the large inline query.The query is lengthy and constructed inline, which can reduce readability. Consider extracting it into a separate function or template file to enhance maintainability and reuse.
73-87: Validate JSON decoding and handle potential errors.When decoding the response with
Jason.decode(body), consider handling decode failures more explicitly rather than returning the rawrespin theelseclause. For example, you could return a structured error tuple or raise an exception to ensure the calling code can properly react to decode failures.server/lib/orcasite/radio/bout.ex (1)
121-136: Possible duplication increateandupdateactions.Both actions recalculate
durationin a similar way. Factor out this logic into a shared change or helper function to reduce repetition and potential maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
server/mix.lockis excluded by!**/*.lockui/package-lock.jsonis excluded by!**/package-lock.jsonui/src/graphql/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (55)
server/config/config.exs(1 hunks)server/config/dev.exs(1 hunks)server/config/prod.exs(1 hunks)server/config/runtime.exs(2 hunks)server/config/test.exs(1 hunks)server/lib/orcasite/accounts/user.ex(0 hunks)server/lib/orcasite/global_setup.ex(2 hunks)server/lib/orcasite/notifications/workers/send_notification_email.ex(1 hunks)server/lib/orcasite/radio/audio_image.ex(3 hunks)server/lib/orcasite/radio/bout.ex(5 hunks)server/lib/orcasite/radio/detection.ex(2 hunks)server/lib/orcasite/radio/feed_segment.ex(1 hunks)server/lib/orcasite/radio/feed_stream.ex(1 hunks)server/lib/orcasite/radio/graphql_client.ex(1 hunks)server/lib/orcasite/radio/workers/generate_spectrogram.ex(2 hunks)server/lib/orcasite/rate_limiter.ex(1 hunks)server/lib/orcasite/repo.ex(1 hunks)server/lib/orcasite/types/audio_image_status.ex(1 hunks)server/lib/orcasite_web.ex(2 hunks)server/lib/orcasite_web/components/core_components.ex(1 hunks)server/lib/orcasite_web/gettext.ex(1 hunks)server/mix.exs(2 hunks)server/priv/repo/migrations/20250127201038_install_ash-uuid_v2_extension.exs(1 hunks)server/priv/repo/migrations/20250127201040_update_start_time_non_null_in_bouts.exs(1 hunks)server/priv/resource_snapshots/repo/bouts/20250127201040.json(1 hunks)server/priv/resource_snapshots/repo/extensions.json(1 hunks)ui/package.json(2 hunks)ui/src/components/Bouts/BaseAudioWidthLayer.tsx(1 hunks)ui/src/components/Bouts/BoutPage.tsx(1 hunks)ui/src/components/Bouts/FeedSegmentsLayer.tsx(1 hunks)ui/src/components/Bouts/FrequencyAxisLayer.tsx(1 hunks)ui/src/components/Bouts/SpectrogramTimeline.tsx(1 hunks)ui/src/components/Bouts/TimelineMarker.tsx(1 hunks)ui/src/components/Bouts/TimelineTickerLayer.tsx(1 hunks)ui/src/components/LoadingSpinner.tsx(1 hunks)ui/src/components/Player/BoutPlayer.tsx(1 hunks)ui/src/components/layouts/SimpleLayout.tsx(2 hunks)ui/src/graphql/fragments/AudioImageParts.graphql(1 hunks)ui/src/graphql/fragments/BoutParts.graphql(1 hunks)ui/src/graphql/fragments/FeedParts.graphql(1 hunks)ui/src/graphql/fragments/FeedSegmentParts.graphql(1 hunks)ui/src/graphql/fragments/FeedStreamParts.graphql(1 hunks)ui/src/graphql/mutations/createBout.graphql(1 hunks)ui/src/graphql/mutations/updateBout.graphql(1 hunks)ui/src/graphql/queries/getBout.graphql(1 hunks)ui/src/graphql/queries/getFeed.graphql(1 hunks)ui/src/graphql/queries/listDetections.graphql(1 hunks)ui/src/graphql/queries/listFeedStreams.graphql(1 hunks)ui/src/hooks/useTimestampFetcher.ts(1 hunks)ui/src/pages/bouts/[boutId].tsx(1 hunks)ui/src/pages/bouts/index.tsx(2 hunks)ui/src/pages/bouts/new/[feedSlug].tsx(1 hunks)ui/src/pages/reports/[candidateId].tsx(2 hunks)ui/src/pages/reports/index.tsx(2 hunks)ui/src/utils/time.ts(1 hunks)
💤 Files with no reviewable changes (1)
- server/lib/orcasite/accounts/user.ex
✅ Files skipped from review due to trivial changes (4)
- ui/src/graphql/fragments/FeedSegmentParts.graphql
- ui/src/components/layouts/SimpleLayout.tsx
- server/priv/resource_snapshots/repo/extensions.json
- server/priv/resource_snapshots/repo/bouts/20250127201040.json
🧰 Additional context used
🪛 GitHub Actions: CI
ui/src/hooks/useTimestampFetcher.ts
[warning] 7-7: exported declaration 'getBucketBase' not used within other modules import/no-unused-modules
ui/src/components/Bouts/BoutPage.tsx
[warning] 68-68: 'log' is assigned a value but never used. Allowed unused vars must match /^_/u. unused-imports/no-unused-vars
ui/src/components/Bouts/SpectrogramTimeline.tsx
[warning] 132-132: React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead. react-hooks/exhaustive-deps
[error] 409-409: Type error: Type '{ timelineStartTime: Date; timelineEndTime: Date; windowStartTime: Date; windowEndTime: Date; pixelsPerMinute: number; zIndex: number; }' is not assignable to type 'IntrinsicAttributes & { timelineStartTime: Date; windowStartTime: Date; windowEndTime: Date; pixelsPerMinute: number; zIndex: number; }'. Property 'timelineEndTime' does not exist on type 'IntrinsicAttributes & { timelineStartTime: Date; windowStartTime: Date; windowEndTime: Date; pixelsPerMinute: number; zIndex: number; }'.
ui/src/pages/bouts/[boutId].tsx
[warning] 11-11: 'targetTime' is assigned a value but never used. Allowed unused vars must match /^_/u. unused-imports/no-unused-vars
ui/src/components/Bouts/FrequencyAxisLayer.tsx
[warning] 12-12: 'scaling' is defined but never used. Allowed unused args must match /^_/u. unused-imports/no-unused-vars
🪛 Biome (1.9.4)
ui/src/components/Player/BoutPlayer.tsx
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 232-233: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 236-237: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (63)
server/lib/orcasite_web/gettext.ex (1)
23-23: LGTM! Good improvement in Gettext configuration.The change from
use Gettexttouse Gettext.Backendexplicitly defines this module as a Gettext backend, which is more precise and follows best practices for internationalization in Phoenix applications.server/lib/orcasite_web/components/core_components.ex (1)
20-20: LGTM! Consistent with the Gettext refactor.The change to
use Gettext, backend: OrcasiteWeb.Gettextaligns with the application-wide standardization of Gettext usage while maintaining the same translation functionality.server/lib/orcasite_web.ex (2)
45-45: LGTM! Consistent Gettext configuration in controllers.The change to
use Gettext, backend: OrcasiteWeb.Gettextin the controller section maintains consistency with the application-wide Gettext refactor.
86-86: LGTM! Consistent Gettext configuration in HTML helpers.The change to
use Gettext, backend: OrcasiteWeb.Gettextin the html_helpers section completes the standardization of Gettext usage across the application.server/config/test.exs (1)
55-55: LGTM!The environment configuration is consistent with the pattern used in other environment files (dev.exs and prod.exs).
ui/src/graphql/fragments/AudioImageParts.graphql (1)
1-12: LGTM!The fragment is well-structured and includes all necessary fields for audio images. It's appropriately used in the listFeedStreams query to retrieve complete audio images.
ui/src/graphql/fragments/FeedStreamParts.graphql (1)
1-12: LGTM! Well-structured fragment definition.The fragment includes all necessary fields for stream metadata and playback information, organized logically.
server/config/dev.exs (1)
117-117: LGTM! Environment configuration is correctly set.The environment is properly set to
:devwhich is consistent with this development configuration file.ui/src/graphql/queries/getBout.graphql (1)
1-8:✅ Verification successful
LGTM! Verify fragment dependencies.
The query is well-structured and properly uses fragments for code reuse.
Let's verify that the required fragments exist:
🏁 Script executed:
#!/bin/bash # Description: Check for required fragment definitions echo "Checking for BoutParts fragment..." fd "BoutParts.graphql" ui/src/graphql/fragments echo "Checking for FeedParts fragment..." fd "FeedParts.graphql" ui/src/graphql/fragmentsLength of output: 339
Action: Fragments Verified
The query is well-structured and all required fragment definitions exist:
- BoutParts fragment:
ui/src/graphql/fragments/BoutParts.graphql- FeedParts fragment:
ui/src/graphql/fragments/FeedParts.graphqlLGTM!
ui/src/graphql/fragments/FeedParts.graphql (1)
1-15:❓ Verification inconclusive
LGTM! Well-structured fragment definition.
The fragment includes all necessary fields for feed metadata, location information, and media URLs.
Let's verify the schema types for URL fields:
🏁 Script executed:
#!/bin/bash # Description: Check schema types for URL fields echo "Checking schema types for URL fields..." ast-grep --pattern 'type Feed { $$$ thumbUrl: $_ imageUrl: $_ mapUrl: $_ $$$ }'Length of output: 180
Action: Confirm correct schema types for URL fields
The GraphQL fragment itself is well-structured and includes all the expected feed properties. However, our attempt to locate the Feed type definition for the URL fields (thumbUrl, imageUrl, mapUrl) using the provided ast-grep query did not return any matches. Please double-check that the Feed type in the GraphQL schema defines these fields with the appropriate types (e.g., URL scalar or string) to ensure consistency.
ui/src/components/LoadingSpinner.tsx (1)
1-9: LGTM! Well-structured loading spinner component.The component is well-implemented with proper TypeScript typing, reusable design, and centered layout. The use of Material-UI components and props spreading allows for good customization options.
server/config/prod.exs (1)
84-84: LGTM! Environment configuration added.The addition of the environment configuration aligns with similar configurations in dev and test environments.
ui/src/graphql/queries/listDetections.graphql (1)
2-14: LGTM! Feed filtering capability added to detections query.The addition of the
feedIdparameter enhances the query's filtering capabilities while maintaining proper GraphQL structure.server/lib/orcasite/types/audio_image_status.ex (1)
2-2: Verify the distinction between:erroredand:failedstates.The addition of the
:erroredstate alongside the existing:failedstate suggests different error scenarios. Please clarify the distinction between these states to ensure proper error handling.Could you explain when an audio image should be marked as
:erroredversus:failed? This will help ensure consistent error handling across the application.ui/src/graphql/queries/getFeed.graphql (1)
3-3: LGTM! Good use of GraphQL fragments.Using the
FeedPartsfragment improves code reusability and maintainability by centralizing the common fields.ui/package.json (1)
30-30:❓ Verification inconclusive
Verify the compatibility of package versions.
The new dependencies look good, but let's verify they are using the latest stable versions and check for any known vulnerabilities.
Also applies to: 35-35, 58-58
🏁 Script executed:
#!/bin/bash # Description: Check for latest versions and vulnerabilities in the new dependencies. # Test: Check npm for latest versions npm view date-fns version npm view lodash version npm view @types/lodash version # Test: Check for known vulnerabilities npm auditLength of output: 469
Version and Vulnerability Check – Manual Verification Needed
The dependencies appear correctly set:
- date-fns is at 4.1.0 (matches the latest).
- lodash is at 4.17.21 (up-to-date).
- @types/lodash is specified as ^4.17.13, while the latest available version is 4.17.15—you may consider bumping this to keep consistency if there’s no breaking change.
Note: The vulnerability check via
npm auditfailed because there isn’t a lockfile present. Please generate one (e.g., withnpm i --package-lock-only) and re-run the audit to ensure there are no vulnerabilities.ui/src/graphql/mutations/createBout.graphql (1)
1-26: LGTM! Well-structured mutation with comprehensive error handling.The mutation is well-designed with:
- Clear parameter definitions
- Optional endTime parameter
- Comprehensive error handling
- Consistent use of the BoutParts fragment
ui/src/pages/bouts/index.tsx (2)
13-13: LGTM!The layout change is consistent with similar changes in other pages, indicating an intentional refactoring of the layout system.
91-91: LGTM!The layout assignment correctly matches the imported layout component.
ui/src/pages/reports/[candidateId].tsx (2)
6-6: LGTM!The layout change aligns with the broader layout refactoring across the application.
72-72: LGTM!The layout assignment correctly matches the imported layout component.
ui/src/graphql/mutations/updateBout.graphql (1)
1-22: LGTM!The mutation is well-structured with:
- Clear parameter definitions
- Comprehensive error handling
- Consistent data structure using BoutParts fragment
server/lib/orcasite/notifications/workers/send_notification_email.ex (1)
42-42: LGTM! Good refactoring.Moving the rate limiting logic to a dedicated module improves code organization and ensures consistent rate limiting across the application.
ui/src/pages/reports/index.tsx (1)
17-17: LGTM! Layout change is consistent.The change to use
getSimpleLayoutaligns with similar changes across other pages, maintaining consistency in the UI.Also applies to: 166-166
server/mix.exs (2)
92-92: LGTM! Switching to a stable version.Moving from a GitHub branch to a stable version of ash_graphql improves dependency stability.
112-113: LGTM! Good additions.The new dependencies enhance the project's capabilities:
- ecto_psql_extras provides useful PostgreSQL monitoring tools
- recase adds string case conversion utilities
server/config/config.exs (1)
94-94: LGTM! Good queue configuration.Adding the audio_images queue with a concurrency of 5 is appropriate for handling image processing tasks without overwhelming the system.
ui/src/pages/bouts/new/[feedSlug].tsx (2)
13-13: Review the hardcoded date.The targetTime is hardcoded to a future date (2025-01-06). Consider making this dynamic or configurable.
39-40: LGTM! Good error handling and component structure.The component properly handles loading states and error cases, and follows a clean structure with appropriate head tags and main content.
Also applies to: 42-57
server/lib/orcasite/radio/detection.ex (2)
54-57: LGTM! Improved relationship formatting.The block syntax for the feed relationship improves code readability.
109-111: LGTM! Well-implemented feed filtering.The feed_id argument and filter are properly implemented, allowing efficient filtering of detections by feed.
server/priv/repo/migrations/20250127201040_update_start_time_non_null_in_bouts.exs (2)
10-16: LGTM! The migration enforces data integrity for the bout page.The changes ensure that bouts always have a category and start time, which is essential for proper display on the new bout page. The removal of the
ongoingcolumn suggests a shift in how bout status is tracked.
18-24: LGTM! Proper rollback functionality is implemented.The
downfunction correctly reverses all changes, making it safe to roll back if needed.ui/src/pages/bouts/[boutId].tsx (1)
23-27: LGTM! Proper error handling is implemented.The component handles all possible states appropriately:
- Loading state with spinner
- Missing bout error
- Missing feed error
server/config/runtime.exs (1)
24-24: LGTM! Host configuration is properly standardized.The changes consistently use "live.orcasound.net" as the default host across both configurations, with proper environment variable override support.
Also applies to: 54-54
ui/src/graphql/queries/listFeedStreams.graphql (2)
7-21: LGTM! Feed streams filtering is well-implemented.The query effectively filters feed streams:
- Handles both active and completed streams
- Uses proper time range filtering
- Includes pagination with sorting
26-40: LGTM! Nested data fetching is properly optimized.The query efficiently fetches related data:
- Feed segments are filtered by time range
- Audio images are filtered by completion status
- Proper use of fragments for field selection
server/priv/repo/migrations/20250127201038_install_ash-uuid_v2_extension.exs (2)
38-46: LGTM! Timestamp extraction is well-implemented.The timestamp extraction function is correctly marked as IMMUTABLE, PARALLEL SAFE, STRICT, and LEAKPROOF, which are appropriate for this deterministic operation.
10-36:❓ Verification inconclusive
Verify UUID v7 implementation against the latest draft RFC.
The implementation looks correct but should be verified against the latest UUID v7 specification.
🌐 Web query:
What is the latest RFC draft for UUID version 7?💡 Result:
The latest specification for UUID version 7 is defined in RFC 9562, which supersedes RFC 4122 and was approved as a Proposed Standard by the Internet Engineering Steering Group (IESG) in November 2023[8][10]. This revision introduces UUIDv7 alongside other modern formats like UUIDv8.
Key Details:
- UUIDv7 Structure: Combines a 48-bit Unix Epoch timestamp (millisecond precision) with 74 bits of randomness split into two blocks[3][4][5]. The layout ensures chronological sorting while maintaining high entropy:
48 bits: Timestamp (ms since 1970-01-01) 4 bits: Version (7) 12 bits: Sub-millisecond data or randomness 2 bits: Variant (RFC4122-compliant) 62 bits: Randomness/counter- Advantages:
- Time-sortable for database indexing efficiency[7]
- Collision resistance comparable to UUIDv4[8]
- No MAC address exposure (unlike UUIDv1)[4]
Implementation Status:
- Officially published as RFC 9562 (formerly
draft-ietf-uuidrev-rfc4122bis)[10]- Supported in libraries like Python's
ramsey/uuid[4][9] and Rust'suuidcrate[5]- Adopted by companies like Buildkite for distributed systems[7]
For developers, UUIDv7 is now stable and recommended over older versions like v1/v6 where possible[3][7].
Citations:
- 1: https://en.wikipedia.org/wiki/Universally_unique_identifier
- 2: https://github.com/stevesimmons/uuid7
- 3: https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format
- 4: https://uuid.ramsey.dev/en/stable/rfc4122/version7.html
- 5: https://discuss.python.org/t/rfc-4122-9562-uuid-version-7-and-8-implementation/56725
- 6: https://github.com/uuid6/uuid6-ietf-draft
- 7: https://buildkite.com/resources/blog/goodbye-integers-hello-uuids/
- 8: https://discuss.python.org/t/add-uuid7-in-uuid-module-in-standard-library/44390
- 9: Have you seen the updated RFC 4122 draft? ramsey/uuid#385
- 10: https://datatracker.ietf.org/doc/draft-ietf-uuidrev-rfc4122bis/14/
Review: UUIDv7 Function Verification Against RFC 9562
The implementation in
server/priv/repo/migrations/20250127201038_install_ash-uuid_v2_extension.exsappears to align with RFC 9562, which defines UUIDv7 as follows:
- 48 bits: Millisecond timestamp (ms since 1970-01-01)
- 4 bits: Version (should be 7)
- 12 bits: Sub-millisecond data (fractional component scaled to 12 bits)
- Remaining bits: Variant and randomness
Implementation Breakdown:
Timestamp Overlay:
The function extracts the millisecond timestamp via
floor(extract(epoch FROM timestamp) * 1000)::BIGINTand usesint8sendwith a substring (starting at byte 3 for 6 bytes) to embed the 48‑bit timestamp. This ensures the upper 6 bytes contain the full timestamp.Sub-millisecond & Version Injection:
The remaining microseconds (i.e. the fractional part from the full microsecond value) are scaled by 4.096 to yield a 12‑bit value. Then, using PostgreSQL’sset_byte:
- Byte at offset 6 (0‑indexed): Is set to a new value formed by concatenating the 4‑bit binary literal
0111(representing version 7) with the upper 4 bits of the 12‑bit fraction.- Byte at offset 7 (0‑indexed): Is replaced with the lower 8 bits of the 12‑bit fractional value.
Randomness and Variant:
The function callsgen_random_uuid()and appliesuuid_send; since only the first 8 bytes are modified (the overlay andset_bytecalls), the randomness and RFC‑compliant variant bits (embedded in the generated UUID) remain intact.Action Items:
- Indexing Verification: Please double-check that the usage of PostgreSQL’s 1‑indexed
overlay(for replacing bytes 1–6) and the 0‑indexedset_byte(modifying bytes at offsets 6 and 7) is correctly coordinated so that the full 48‑bit timestamp remains preserved and the version and sub-millisecond fields are accurately positioned.- Conformance Testing: Ensure that tests confirm the resulting UUIDs adhere to RFC 9562’s structure (48‑bit timestamp, 4‑bit version, 12‑bit sub‑millisecond field, and preserved variant/random).
If these aspects have been manually verified or validated through tests, the implementation is compliant with the latest UUIDv7 draft (RFC 9562).
server/lib/orcasite/global_setup.ex (1)
39-43: LGTM! Clear and concise implementation.The function provides a convenient way to populate recent feed streams with sensible defaults.
server/lib/orcasite/radio/audio_image.ex (4)
81-91: Looks good for a dedicated feed-based query.The pagination configuration and filter expression are clear and straightforward. This read action nicely scopes audio image retrieval to a specific feed.
216-216: New:erroredstatus is consistent.Switching the status from
:failedto:erroredprovides a more precise indication of error states. Ensure that all consuming code or downstream logic is updated accordingly.
230-233: Verify the distinction between:failedand:errored.Two separate statuses (
:erroredvs.:failed) may be confusing unless there's a clear semantic difference. Confirm whether both are needed or if one can be consolidated.Would you like to check all references to
:failedand:erroredin the codebase to ensure they are intentionally distinct?
238-238: GraphQL configuration for listing audio images by feed.Declaring the new query
:audio_images, :for_feedprovides a convenient entry point for clients to retrieve feed-specific audio images. This looks good to me.Also applies to: 240-242
server/lib/orcasite/radio/workers/generate_spectrogram.ex (1)
3-3: Queue renamed from:feedsto:audio_images.This aligns well with the specialized queue introduced for audio images. Ensure the queue is correctly declared in the config and monitored in production.
ui/src/components/Bouts/BoutPage.tsx (2)
581-616: Category icon helper is clear and user-friendly.Using icons for various categories is intuitive. The approach of returning different images based on the
"BIOPHONY","ANTHROPHONY", or"GEOPHONY"states is well-structured.
618-636: Tab panel component is well-implemented.A straightforward pattern for conditionally rendering tab content. This promotes reusability across other parts of the UI if needed.
server/lib/orcasite/radio/graphql_client.ex (2)
1-12: Consider using variables instead of string interpolation for GraphQL queries.String interpolation within the query (e.g.,
feed(slug: "#{feed_slug}")) may lead to potential injection vulnerabilities iffeed_slugoriginates from user input. GraphQL typically supports using variables to avoid direct interpolation.
89-98: Confirm environment key naming.This function references
Application.get_env(:orcasite, :prod_host). Ensure that this environment configuration is always present and references the correct environment host, even in non-production environments. Otherwise, consider a fallback or environment-based logic.ui/src/components/Bouts/SpectrogramTimeline.tsx (1)
213-231: Reassess the frequentsetIntervalapproach.Centering the window on the current time every 100ms can be resource-intensive. Consider alternatives, such as event-based updates from the player to reduce overhead and complexity.
[performance]server/lib/orcasite/rate_limiter.ex (2)
2-6: Verify key conversion logic when using tuples and lists.You convert tuples to lists and lists to joined strings (e.g.,
Enum.join(keys, ":")). Ensure the resulting key is unique enough to distinguish different tuples/lists, especially if “:” can appear in original data.
8-10: Check for potential naming conflicts with Atom keys.When converting atoms to strings (
Atom.to_string(key)), confirm that collisions with string-based keys won't occur. This is especially important if you rely on distinct naming.server/lib/orcasite/radio/bout.ex (11)
23-23: Double-check non-nil constraints.By disallowing
nilforstart_timeandcategory, you ensure these fields must always be provided. Verify there's no valid scenario that requires them to be absent. If so, consider providing defaults or modifying business logic.Also applies to: 29-29
39-41: Review public exposure offeedrelationship.Enabling
public?on this belongs_to relationship can expose sensitive feed data. Confirm that the resulting GraphQL or JSON API responses won't inadvertently disclose confidential information.
70-70: Change in default actions.Removing the
:updateaction from the default set is aligned with your customupdateaction. Verify that any existing consumers of the default:updateaction aren’t affected.
81-81: Conditional filter usage.Conditionally filtering by
feed_idis straightforward and efficient. Approaches like this effectively limit results whenfeed_idis provided and return all records otherwise.
86-87: Clarity on accepted attributes during creation.Accepting
:category,:start_time, and:end_timeduring creation is consistent with your domain constraints (e.g., non-nilstart_time). This change aligns well with the resource’s data requirements.
88-89: Mandatoryfeed_idargument.Requiring a non-nil
feed_idensures that every bout is associated with a feed, enforcing referential integrity. This appears logically sound.
90-97: Ensure relationship validity.Using
Ash.Changeset.manage_relationship(:feed, …)to link thefeed_idargument to the feed relationship is correct. Consider how the system handles invalid feed IDs—Ash will error out, but confirm this behavior is acceptable.
99-109: Duration calculation logic.Computing duration with
DateTime.diff(end_time, start_time, :millisecond) / 1000is reasonable. However, ifstart_timeis ever afterend_time, this could yield negative values. Confirm whether that scenario is permissible or if further validation is needed.
141-141: GraphQL attribute types set to:id.Declaring
feed_idandfeed_stream_idas ID types aligns with referencing external resources in your GraphQL schema. This remains consistent with existing conventions.
145-145: Addedget :boutquery.Introducing a specific query for retrieving a single bout by ID is a common GraphQL best practice. This provides a clear and efficient access pattern.
150-150: Addedupdate_boutmutation.Publishing the custom
updateaction as a GraphQL mutation neatly integrates with your resource’s security policies. Make sure to test it thoroughly for authorization checks and data validation.
| fragment BoutParts on Bout { | ||
| id | ||
| category | ||
| duration | ||
| endTime | ||
| startTime | ||
| endTime | ||
| } |
There was a problem hiding this comment.
Remove duplicate endTime field.
The endTime field appears twice in the fragment. Please remove the duplicate at line 7.
fragment BoutParts on Bout {
id
category
duration
endTime
startTime
- endTime
}📝 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.
| fragment BoutParts on Bout { | |
| id | |
| category | |
| duration | |
| endTime | |
| startTime | |
| endTime | |
| } | |
| fragment BoutParts on Bout { | |
| id | |
| category | |
| duration | |
| endTime | |
| startTime | |
| } |
ui/src/hooks/useTimestampFetcher.ts
Outdated
| export const getBucketBase = (bucket: string) => | ||
| `https://${bucket}.s3.amazonaws.com`; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the usage of the exported function.
The function is now exported but the linter indicates it's not being used by other modules. Please verify if this export is necessary for the new bout page components.
Run the following script to verify the function usage:
🏁 Script executed:
#!/bin/bash
# Description: Verify if getBucketBase is imported and used in other files.
# Test: Search for imports of getBucketBase. Expect: Multiple imports in Bouts components.
rg -A 5 $'import.*getBucketBase.*from.*useTimestampFetcher'
# Test: Search for usage of getBucketBase. Expect: Multiple usages in Bouts components.
rg -A 5 $'getBucketBase\('Length of output: 914
Action Required: Verify and Adjust the Export of getBucketBase
Our investigation shows that getBucketBase is only being used within the same file (specifically in the construction of URLs for both latest.txt and the HLS stream) and is not imported elsewhere in the codebase. This is likely why the linter is flagging it as an unused export. Please confirm if this function is indeed intended for external use (for example, by new bout page components). If not, consider removing the export and making it a local helper function to resolve the linter warning.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 7-7: exported declaration 'getBucketBase' not used within other modules import/no-unused-modules
ui/src/pages/bouts/[boutId].tsx
Outdated
| const BoutShowPage: NextPageWithLayout = () => { | ||
| const targetTime = new Date("2024-12-11 19:55:44.013Z"); |
There was a problem hiding this comment.
Remove unused targetTime variable.
The targetTime variable is defined but never used, as flagged by the CI pipeline.
Apply this diff to fix the issue:
-const targetTime = new Date("2024-12-11 19:55:44.013Z");📝 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.
| const BoutShowPage: NextPageWithLayout = () => { | |
| const targetTime = new Date("2024-12-11 19:55:44.013Z"); | |
| const BoutShowPage: NextPageWithLayout = () => { | |
| // The unused `targetTime` variable has been removed. |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 11-11: 'targetTime' is assigned a value but never used. Allowed unused vars must match /^_/u. unused-imports/no-unused-vars
| <TimelineTickerLayer | ||
| timelineStartTime={timelineStartTime} | ||
| timelineEndTime={timelineEndTime} | ||
| windowStartTime={windowStartTime} | ||
| windowEndTime={windowEndTime} | ||
| pixelsPerMinute={pixelsPerMinute} | ||
| zIndex={3} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Remove or rename the unused prop timelineEndTime to avoid type errors.
The pipeline indicates a TypeScript error at line 409 regarding timelineEndTime. The TimelineTickerLayer component’s prop type does not accept a timelineEndTime. Either remove that prop if unused or update the child component’s prop definition accordingly.
Proposed fix:
- timelineEndTime={timelineEndTime}📝 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.
| <TimelineTickerLayer | |
| timelineStartTime={timelineStartTime} | |
| timelineEndTime={timelineEndTime} | |
| windowStartTime={windowStartTime} | |
| windowEndTime={windowEndTime} | |
| pixelsPerMinute={pixelsPerMinute} | |
| zIndex={3} | |
| /> | |
| )} | |
| <TimelineTickerLayer | |
| timelineStartTime={timelineStartTime} | |
| windowStartTime={windowStartTime} | |
| windowEndTime={windowEndTime} | |
| pixelsPerMinute={pixelsPerMinute} | |
| zIndex={3} | |
| /> | |
| )} |
🧰 Tools
🪛 GitHub Actions: CI
[error] 409-409: Type error: Type '{ timelineStartTime: Date; timelineEndTime: Date; windowStartTime: Date; windowEndTime: Date; pixelsPerMinute: number; zIndex: number; }' is not assignable to type 'IntrinsicAttributes & { timelineStartTime: Date; windowStartTime: Date; windowEndTime: Date; pixelsPerMinute: number; zIndex: number; }'. Property 'timelineEndTime' does not exist on type 'IntrinsicAttributes & { timelineStartTime: Date; windowStartTime: Date; windowEndTime: Date; pixelsPerMinute: number; zIndex: number; }'.
| const setWindowStartTime = useCallback( | ||
| _.throttle(setWindowStartTimeUnthrottled, 500, { trailing: false }), | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Include dependencies in the useCallback hook to avoid stale closures.
Your pipeline flagged a warning for react-hooks/exhaustive-deps regarding the useCallback usage for throttling. Make sure to list every variable or function used inside the callback in the dependency array to prevent potential bugs.
Consider adjusting as follows:
-const setWindowStartTime = useCallback(
- _.throttle(setWindowStartTimeUnthrottled, 500, { trailing: false }),
- [],
-);
+const setWindowStartTime = useCallback(() => {
+ return _.throttle(setWindowStartTimeUnthrottled, 500, { trailing: false })();
+}, [setWindowStartTimeUnthrottled]);Adjust the approach as needed to ensure the throttle function is not recreated incorrectly or capturing stale data.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 132-132: React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead. react-hooks/exhaustive-deps
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/src/components/Bouts/SpectrogramTimeline.tsx (1)
132-146:⚠️ Potential issueFix throttling implementation to prevent stale closures.
The current throttling implementation may lead to stale closures. The throttled functions should be memoized with their dependencies.
-const setWindowStartTime = useCallback( - _.throttle(setWindowStartTimeUnthrottled, 500, { trailing: false }), - [], -); +const setWindowStartTime = useCallback(() => { + return _.throttle(setWindowStartTimeUnthrottled, 500, { trailing: false })(); +}, [setWindowStartTimeUnthrottled]); -const setWindowEndTime = useCallback( - _.throttle(setWindowEndTimeUnthrottled, 500, { trailing: false }), - [], -); +const setWindowEndTime = useCallback(() => { + return _.throttle(setWindowEndTimeUnthrottled, 500, { trailing: false })(); +}, [setWindowEndTimeUnthrottled]);
🧹 Nitpick comments (5)
ui/src/components/Bouts/BoutPage.tsx (2)
62-107: Consider adding prop validation fortargetTimeandtargetAudioCategory.The component accepts optional props but doesn't validate their values. Consider adding validation to ensure
targetTimeis a valid date andtargetAudioCategoryis one of the allowed values.+const isValidDate = (date?: Date) => date instanceof Date && !isNaN(date.getTime()); + export default function BoutPage({ isNew, feed, targetAudioCategory, targetTime, bout, }: { isNew: boolean; feed: FeedQuery["feed"]; targetAudioCategory?: AudioCategory; - targetTime?: Date; + targetTime?: Date | null; bout?: BoutQuery["bout"]; }) { + if (targetTime && !isValidDate(targetTime)) { + console.warn('Invalid targetTime provided to BoutPage'); + targetTime = null; + } + + if (targetAudioCategory && !audioCategories.includes(targetAudioCategory)) { + console.warn('Invalid targetAudioCategory provided to BoutPage'); + targetAudioCategory = undefined; + }
638-693: Consider extractingCategoryIconandTabPanelto separate files.These helper components should be moved to their own files to improve code organization and maintainability.
Move these components to:
ui/src/components/Bouts/CategoryIcon.tsxui/src/components/Common/TabPanel.tsxui/src/components/Bouts/TimelineTickerLayer.tsx (2)
33-80: Consider extracting scale constants and calculations to a separate utility.The scale calculations and constants could be moved to a separate utility file for better reusability and testing.
Create a new file
ui/src/utils/timelineScales.ts:export const SCALE_TYPES = { HOURS: { DAY: 24, QUARTER_DAY: 6, EIGHTH_DAY: 3, HOUR: 1 }, MINUTES: { TEN: 10, FIVE: 5, ONE: 1 }, SECONDS: { THIRTY: 30, TEN: 10, FIVE: 5, ONE: 1, TENTH: 0.1, HUNDREDTH: 0.01 } } as const; export function calculateTimelineScales(windowRange: number, ticksPerWindow: number) { const tickSize = windowRange / ticksPerWindow; return { tickScales: generateScales(), labelScales: generateScales(), findMinScaleIndex: (scales: number[]) => scales.findIndex(num => Math.floor(tickSize / num) > 0) }; }
81-118: Add memoization for time window calculations.The time window calculations are being recomputed on every render. Consider memoizing these values.
+const useTimeWindowCalculations = ( + windowStartTime: Date, + windowEndTime: Date, + windowRangeBufferFactor: number, + scale: number +) => { + return useMemo(() => { + const tickStartTime = roundToNearest( + subMilliseconds(windowStartTime, windowRange * windowRangeBufferFactor), + scale, + "ceil" + ); + const tickEndTime = roundToNearest( + addMilliseconds(windowEndTime, windowRange * windowRangeBufferFactor), + scale, + "ceil" + ); + return { tickStartTime, tickEndTime }; + }, [windowStartTime, windowEndTime, windowRangeBufferFactor, scale]); +};ui/src/components/Bouts/SpectrogramTimeline.tsx (1)
243-325: Consider extracting drag handling logic to a custom hook.The touch and mouse event handling logic is complex and could be extracted to a custom hook for better reusability and testing.
Create a new file
ui/src/hooks/useTimelineDrag.ts:export function useTimelineDrag({ spectrogramWindow, timelineStartTime, pixelsPerMinute, setWindowStartTime, setWindowEndTime, playerControls, }) { const [isDragging, setIsDragging] = useState(false); const windowStartX = useRef(0); const windowScrollX = useRef(0); const handleDragStart = useCallback((pageX: number) => { setIsDragging(true); windowStartX.current = pageX - (spectrogramWindow.current?.offsetLeft ?? 0); windowScrollX.current = spectrogramWindow.current?.scrollLeft ?? 0; playerControls?.pause(); }, [spectrogramWindow, playerControls]); // ... rest of the drag handling logic return { isDragging, handleTouchStart, handleTouchMove, handleTouchEnd, handleMouseDown, handleMouseMove, handleMouseUp, handleMouseLeave, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/src/components/Bouts/BoutPage.tsx(1 hunks)ui/src/components/Bouts/SpectrogramTimeline.tsx(1 hunks)ui/src/components/Bouts/TimelineTickerLayer.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
ui/src/components/Bouts/BoutPage.tsx
Outdated
| const [boutForm, setBoutForm] = useState<{ | ||
| errors: Record<string, string>; | ||
| }>({ | ||
| errors: {}, | ||
| }); | ||
| const createBoutMutation = useCreateBoutMutation({ | ||
| onSuccess: ({ createBout: { errors, result } }) => { | ||
| if (errors && errors.length > 0) { | ||
| console.error(errors); | ||
| setBoutForm((form) => ({ | ||
| ...form, | ||
| errors: { | ||
| ...form.errors, | ||
| ...Object.fromEntries( | ||
| errors.map(({ code, message }) => [code, message] as const), | ||
| ), | ||
| }, | ||
| })); | ||
| } else if (result) { | ||
| router.push(`/bouts/${result.id}`); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| const updateBoutMutation = useUpdateBoutMutation({ | ||
| onSuccess: ({ updateBout: { errors } }) => { | ||
| if (errors && errors.length > 0) { | ||
| console.error(errors); | ||
| setBoutForm((form) => ({ | ||
| ...form, | ||
| errors: { | ||
| ...form.errors, | ||
| ...Object.fromEntries( | ||
| errors.map(({ code, message }) => [code, message] as const), | ||
| ), | ||
| }, | ||
| })); | ||
| } else { | ||
| setBoutSaved(true); | ||
| setTimeout(() => { | ||
| setBoutSaved(false); | ||
| }, 5000); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| const saveBout = () => { | ||
| setBoutForm((form) => ({ ...form, errors: {} })); | ||
| if (audioCategory && boutStartTime) { | ||
| if (isNew) { | ||
| createBoutMutation.mutate({ | ||
| feedId: feed.id, | ||
| startTime: boutStartTime, | ||
| endTime: boutEndTime, | ||
| category: audioCategory, | ||
| }); | ||
| } else if (bout) { | ||
| updateBoutMutation.mutate({ | ||
| id: bout.id, | ||
| startTime: boutStartTime, | ||
| endTime: boutEndTime, | ||
| category: audioCategory, | ||
| }); | ||
| } | ||
| } else { | ||
| const errors: Record<string, string> = {}; | ||
| if (!audioCategory) { | ||
| errors["audioCategory"] = "Audio category required"; | ||
| } | ||
| if (!boutStartTime) { | ||
| errors["startTime"] = "Bout start time required"; | ||
| } | ||
| setBoutForm((form) => ({ ...form, errors })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in the saveBout function.
The error handling could be improved by:
- Adding type safety for error codes
- Handling network errors
- Adding loading state management
+type BoutFormErrors = {
+ audioCategory?: string;
+ startTime?: string;
+ network?: string;
+};
+
const [boutForm, setBoutForm] = useState<{
- errors: Record<string, string>;
+ errors: BoutFormErrors;
+ isLoading: boolean;
}>({
errors: {},
+ isLoading: false,
});
const saveBout = () => {
setBoutForm((form) => ({ ...form, errors: {} }));
if (audioCategory && boutStartTime) {
+ setBoutForm((form) => ({ ...form, isLoading: true }));
if (isNew) {
createBoutMutation.mutate({
feedId: feed.id,
startTime: boutStartTime,
endTime: boutEndTime,
category: audioCategory,
+ }, {
+ onError: (error) => {
+ setBoutForm((form) => ({
+ ...form,
+ isLoading: false,
+ errors: { network: error.message }
+ }));
+ }
});
} else if (bout) {
// Similar changes for updateBoutMutation
}
} else {
- const errors: Record<string, string> = {};
+ const errors: BoutFormErrors = {};
if (!audioCategory) {
errors["audioCategory"] = "Audio category required";
}
if (!boutStartTime) {
errors["startTime"] = "Bout start time required";
}
setBoutForm((form) => ({ ...form, errors }));
}
};📝 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.
| const [boutForm, setBoutForm] = useState<{ | |
| errors: Record<string, string>; | |
| }>({ | |
| errors: {}, | |
| }); | |
| const createBoutMutation = useCreateBoutMutation({ | |
| onSuccess: ({ createBout: { errors, result } }) => { | |
| if (errors && errors.length > 0) { | |
| console.error(errors); | |
| setBoutForm((form) => ({ | |
| ...form, | |
| errors: { | |
| ...form.errors, | |
| ...Object.fromEntries( | |
| errors.map(({ code, message }) => [code, message] as const), | |
| ), | |
| }, | |
| })); | |
| } else if (result) { | |
| router.push(`/bouts/${result.id}`); | |
| } | |
| }, | |
| }); | |
| const updateBoutMutation = useUpdateBoutMutation({ | |
| onSuccess: ({ updateBout: { errors } }) => { | |
| if (errors && errors.length > 0) { | |
| console.error(errors); | |
| setBoutForm((form) => ({ | |
| ...form, | |
| errors: { | |
| ...form.errors, | |
| ...Object.fromEntries( | |
| errors.map(({ code, message }) => [code, message] as const), | |
| ), | |
| }, | |
| })); | |
| } else { | |
| setBoutSaved(true); | |
| setTimeout(() => { | |
| setBoutSaved(false); | |
| }, 5000); | |
| } | |
| }, | |
| }); | |
| const saveBout = () => { | |
| setBoutForm((form) => ({ ...form, errors: {} })); | |
| if (audioCategory && boutStartTime) { | |
| if (isNew) { | |
| createBoutMutation.mutate({ | |
| feedId: feed.id, | |
| startTime: boutStartTime, | |
| endTime: boutEndTime, | |
| category: audioCategory, | |
| }); | |
| } else if (bout) { | |
| updateBoutMutation.mutate({ | |
| id: bout.id, | |
| startTime: boutStartTime, | |
| endTime: boutEndTime, | |
| category: audioCategory, | |
| }); | |
| } | |
| } else { | |
| const errors: Record<string, string> = {}; | |
| if (!audioCategory) { | |
| errors["audioCategory"] = "Audio category required"; | |
| } | |
| if (!boutStartTime) { | |
| errors["startTime"] = "Bout start time required"; | |
| } | |
| setBoutForm((form) => ({ ...form, errors })); | |
| } | |
| }; | |
| type BoutFormErrors = { | |
| audioCategory?: string; | |
| startTime?: string; | |
| network?: string; | |
| }; | |
| const [boutForm, setBoutForm] = useState<{ | |
| errors: BoutFormErrors; | |
| isLoading: boolean; | |
| }>({ | |
| errors: {}, | |
| isLoading: false, | |
| }); | |
| const createBoutMutation = useCreateBoutMutation({ | |
| onSuccess: ({ createBout: { errors, result } }) => { | |
| if (errors && errors.length > 0) { | |
| console.error(errors); | |
| setBoutForm((form) => ({ | |
| ...form, | |
| errors: { | |
| ...form.errors, | |
| ...Object.fromEntries( | |
| errors.map(({ code, message }) => [code, message] as const), | |
| ), | |
| }, | |
| })); | |
| } else if (result) { | |
| router.push(`/bouts/${result.id}`); | |
| } | |
| }, | |
| }); | |
| const updateBoutMutation = useUpdateBoutMutation({ | |
| onSuccess: ({ updateBout: { errors } }) => { | |
| if (errors && errors.length > 0) { | |
| console.error(errors); | |
| setBoutForm((form) => ({ | |
| ...form, | |
| errors: { | |
| ...form.errors, | |
| ...Object.fromEntries( | |
| errors.map(({ code, message }) => [code, message] as const), | |
| ), | |
| }, | |
| })); | |
| } else { | |
| setBoutSaved(true); | |
| setTimeout(() => { | |
| setBoutSaved(false); | |
| }, 5000); | |
| } | |
| }, | |
| }); | |
| const saveBout = () => { | |
| setBoutForm((form) => ({ ...form, errors: {} })); | |
| if (audioCategory && boutStartTime) { | |
| setBoutForm((form) => ({ ...form, isLoading: true })); | |
| if (isNew) { | |
| createBoutMutation.mutate( | |
| { | |
| feedId: feed.id, | |
| startTime: boutStartTime, | |
| endTime: boutEndTime, | |
| category: audioCategory, | |
| }, | |
| { | |
| onError: (error) => { | |
| setBoutForm((form) => ({ | |
| ...form, | |
| isLoading: false, | |
| errors: { network: error.message }, | |
| })); | |
| }, | |
| } | |
| ); | |
| } else if (bout) { | |
| updateBoutMutation.mutate({ | |
| id: bout.id, | |
| startTime: boutStartTime, | |
| endTime: boutEndTime, | |
| category: audioCategory, | |
| }); | |
| } | |
| } else { | |
| const errors: BoutFormErrors = {}; | |
| if (!audioCategory) { | |
| errors["audioCategory"] = "Audio category required"; | |
| } | |
| if (!boutStartTime) { | |
| errors["startTime"] = "Bout start time required"; | |
| } | |
| setBoutForm((form) => ({ ...form, errors })); | |
| } | |
| }; |
| function centerWindow( | ||
| spectrogramWindow: MutableRefObject<HTMLDivElement | null>, | ||
| targetTime: Date, | ||
| timelineStartTime: Date, | ||
| pixelsPerMinute: number, | ||
| setWindowStartTime: (value: SetStateAction<Date | undefined>) => void, | ||
| setWindowEndTime: (value: SetStateAction<Date | undefined>) => void, | ||
| playerControls?: PlayerControls, | ||
| ) { | ||
| if (spectrogramWindow.current) { | ||
| const offset = timeToOffset(targetTime, timelineStartTime, pixelsPerMinute); | ||
| spectrogramWindow.current.scrollLeft = | ||
| offset - spectrogramWindow.current.clientWidth / 2; | ||
|
|
||
| if (spectrogramWindow.current) { | ||
| setWindowStartTime( | ||
| offsetToTime( | ||
| spectrogramWindow.current.scrollLeft, | ||
| timelineStartTime, | ||
| pixelsPerMinute, | ||
| ), | ||
| ); | ||
| setWindowEndTime( | ||
| offsetToTime( | ||
| spectrogramWindow.current.scrollLeft + | ||
| spectrogramWindow.current.clientWidth, | ||
| timelineStartTime, | ||
| pixelsPerMinute, | ||
| ), | ||
| ); | ||
| } | ||
| if (playerControls?.setPlayerTime) { | ||
| playerControls.setPlayerTime(targetTime); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid time values in centerWindow.
The centerWindow function should validate time values to prevent potential runtime errors.
function centerWindow(
spectrogramWindow: MutableRefObject<HTMLDivElement | null>,
targetTime: Date,
timelineStartTime: Date,
pixelsPerMinute: number,
setWindowStartTime: (value: SetStateAction<Date | undefined>) => void,
setWindowEndTime: (value: SetStateAction<Date | undefined>) => void,
playerControls?: PlayerControls,
) {
+ if (!(targetTime instanceof Date) || isNaN(targetTime.getTime())) {
+ console.error('Invalid targetTime provided to centerWindow');
+ return;
+ }
+
+ if (!(timelineStartTime instanceof Date) || isNaN(timelineStartTime.getTime())) {
+ console.error('Invalid timelineStartTime provided to centerWindow');
+ return;
+ }
+
if (spectrogramWindow.current) {
const offset = timeToOffset(targetTime, timelineStartTime, pixelsPerMinute);
// ... rest of the function
}
}📝 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.
| function centerWindow( | |
| spectrogramWindow: MutableRefObject<HTMLDivElement | null>, | |
| targetTime: Date, | |
| timelineStartTime: Date, | |
| pixelsPerMinute: number, | |
| setWindowStartTime: (value: SetStateAction<Date | undefined>) => void, | |
| setWindowEndTime: (value: SetStateAction<Date | undefined>) => void, | |
| playerControls?: PlayerControls, | |
| ) { | |
| if (spectrogramWindow.current) { | |
| const offset = timeToOffset(targetTime, timelineStartTime, pixelsPerMinute); | |
| spectrogramWindow.current.scrollLeft = | |
| offset - spectrogramWindow.current.clientWidth / 2; | |
| if (spectrogramWindow.current) { | |
| setWindowStartTime( | |
| offsetToTime( | |
| spectrogramWindow.current.scrollLeft, | |
| timelineStartTime, | |
| pixelsPerMinute, | |
| ), | |
| ); | |
| setWindowEndTime( | |
| offsetToTime( | |
| spectrogramWindow.current.scrollLeft + | |
| spectrogramWindow.current.clientWidth, | |
| timelineStartTime, | |
| pixelsPerMinute, | |
| ), | |
| ); | |
| } | |
| if (playerControls?.setPlayerTime) { | |
| playerControls.setPlayerTime(targetTime); | |
| } | |
| } | |
| } | |
| function centerWindow( | |
| spectrogramWindow: MutableRefObject<HTMLDivElement | null>, | |
| targetTime: Date, | |
| timelineStartTime: Date, | |
| pixelsPerMinute: number, | |
| setWindowStartTime: (value: SetStateAction<Date | undefined>) => void, | |
| setWindowEndTime: (value: SetStateAction<Date | undefined>) => void, | |
| playerControls?: PlayerControls, | |
| ) { | |
| if (!(targetTime instanceof Date) || isNaN(targetTime.getTime())) { | |
| console.error('Invalid targetTime provided to centerWindow'); | |
| return; | |
| } | |
| if (!(timelineStartTime instanceof Date) || isNaN(timelineStartTime.getTime())) { | |
| console.error('Invalid timelineStartTime provided to centerWindow'); | |
| return; | |
| } | |
| if (spectrogramWindow.current) { | |
| const offset = timeToOffset(targetTime, timelineStartTime, pixelsPerMinute); | |
| spectrogramWindow.current.scrollLeft = | |
| offset - spectrogramWindow.current.clientWidth / 2; | |
| if (spectrogramWindow.current) { | |
| setWindowStartTime( | |
| offsetToTime( | |
| spectrogramWindow.current.scrollLeft, | |
| timelineStartTime, | |
| pixelsPerMinute, | |
| ), | |
| ); | |
| setWindowEndTime( | |
| offsetToTime( | |
| spectrogramWindow.current.scrollLeft + | |
| spectrogramWindow.current.clientWidth, | |
| timelineStartTime, | |
| pixelsPerMinute, | |
| ), | |
| ); | |
| } | |
| if (playerControls?.setPlayerTime) { | |
| playerControls.setPlayerTime(targetTime); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/src/components/Bouts/BoutPage.tsx (1)
197-271: 🛠️ Refactor suggestionPrevious error handling improvements are still needed.
The error handling in the
saveBoutfunction could be improved as suggested in the previous review.Please refer to the previous review comment for the suggested improvements to error handling, including type safety for error codes, network error handling, and loading state management.
🧹 Nitpick comments (3)
ui/src/pages/bouts/new/[feedSlug].tsx (1)
17-31: Consider extracting audio categories to a shared constant.The audio categories array is defined inline but appears to be used in multiple places (also in
BoutPage.tsx).Extract this to a shared constant to maintain consistency and reduce duplication:
+// In a shared constants file (e.g., src/constants/audio.ts) +export const AUDIO_CATEGORIES = ["ANTHROPHONY", "BIOPHONY", "GEOPHONY"] as const; +export type AudioCategory = typeof AUDIO_CATEGORIES[number]; - const audioCategories: AudioCategory[] = [ - "ANTHROPHONY", - "BIOPHONY", - "GEOPHONY", - ] as const; + const audioCategories = AUDIO_CATEGORIES;ui/src/components/Bouts/BoutPage.tsx (2)
639-674: Add error boundary for image loading failures.The
CategoryIconcomponent doesn't handle image loading failures, which could affect the UI if the icon assets fail to load.Add error handling for image loading:
function CategoryIcon({ audioCategory, size, onError, }: { audioCategory: AudioCategory; size?: number; + onError?: () => void; }) { size = size ?? 15; if (audioCategory === "BIOPHONY") return ( <Image src={whaleFlukeIconImage.src} width={size} height={size} alt="Whale fluke icon" + onError={onError} + fallback={<GraphicEq />} /> ); // Apply similar changes to other image components }
676-694: Add prop types validation for TabPanel component.The
TabPanelcomponent could benefit from prop types validation to ensure required props are provided.Add prop types validation:
+import PropTypes from 'prop-types'; function TabPanel(props: { children?: React.ReactNode; index: number; value: number; }) { const { children, value, index, ...other } = props; return ( <div role="tabpanel" hidden={value !== index} id={`simple-tabpanel-${index}`} aria-labelledby={`simple-tab-${index}`} {...other} > {value === index && <Box sx={{ p: 3 }}>{children}</Box>} </div> ); } +TabPanel.propTypes = { + children: PropTypes.node, + index: PropTypes.number.isRequired, + value: PropTypes.number.isRequired, +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/src/components/Bouts/BoutPage.tsx(1 hunks)ui/src/pages/bouts/new/[feedSlug].tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
ui/src/pages/bouts/new/[feedSlug].tsx (1)
12-12: Verify the hardcoded target time.The component uses a hardcoded target time set to January 6, 2025. This might cause issues when the date passes.
Consider making this dynamic or documenting why this specific date is used.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
ui/src/pages/bouts/new/[feedSlug].tsx (1)
14-27: Add validation for audio category mapping.While the code properly validates if the category exists, it doesn't handle case sensitivity well. Consider using a more robust approach:
- if ( - categoryParam && - audioCategories.includes(_.toUpper(categoryParam) as AudioCategory) - ) { - targetAudioCategory = categoryParam.toUpperCase() as AudioCategory; + if (categoryParam) { + const normalizedCategory = _.toUpper(categoryParam); + if (audioCategories.includes(normalizedCategory as AudioCategory)) { + targetAudioCategory = normalizedCategory as AudioCategory; + }ui/src/components/Bouts/BoutPage.tsx (4)
105-124: Consider extracting timeline calculation logic to a utility function.The timeline start/end calculation logic is complex and would be more maintainable if extracted to a separate utility function. This would make the component cleaner and the logic more testable.
// Add to /src/utils/time.ts or a similar file +export function calculateTimelineRange(targetTime: Date, now: Date, timeBuffer: number, nearestMinutes: number) { + const startTime = roundToNearest( + subMinutes(targetTime, timeBuffer), + nearestMinutes * 60 * 1000, + "floor" + ); + + const endTime = min([ + now, + roundToNearest( + max([targetTime, addMinutes(targetTime, timeBuffer)]), + nearestMinutes * 60 * 1000, + "ceil" + ), + ]); + + return { startTime, endTime }; +} // In your component +const { startTime: timelineStartTime, endTime: timelineEndTime } = calculateTimelineRange( + targetTime, + now, + timeBuffer, + nearestMinutes +); -const [timelineStartTime, setTimelineStartTime] = useState<Date>( - roundToNearest( - subMinutes(targetTime, timeBuffer), - nearestMinutes * 60 * 1000, - "floor", - ), -); -const [timelineEndTime, setTimelineEndTime] = useState<Date>( - min([ - now, - roundToNearest( - max([targetTime, addMinutes(targetTime, timeBuffer)]), - nearestMinutes * 60 * 1000, - "ceil", - ), - ]), -); +const [timelineStartTime, setTimelineStartTime] = useState<Date>(startTime); +const [timelineEndTime, setTimelineEndTime] = useState<Date>(endTime);
635-670: Consider using a map for CategoryIcon component.The
CategoryIconcomponent uses if-else logic for icon selection. Consider using an object map for cleaner code:function CategoryIcon({ audioCategory, size, }: { audioCategory: AudioCategory; size?: number; }) { size = size ?? 15; - if (audioCategory === "BIOPHONY") - return ( - <Image - src={whaleFlukeIconImage.src} - width={size} - height={size} - alt="Whale fluke icon" - /> - ); - if (audioCategory === "ANTHROPHONY") - return ( - <Image - src={vesselIconImage.src} - width={size} - height={size} - alt="Vessel icon" - /> - ); - if (audioCategory === "GEOPHONY") - return ( - <Image - src={wavesIconImage.src} - width={size} - height={size} - alt="Waves icon" - /> - ); + const iconMap = { + BIOPHONY: { src: whaleFlukeIconImage.src, alt: "Whale fluke icon" }, + ANTHROPHONY: { src: vesselIconImage.src, alt: "Vessel icon" }, + GEOPHONY: { src: wavesIconImage.src, alt: "Waves icon" }, + }; + + const icon = iconMap[audioCategory]; + if (!icon) return null; + + return ( + <Image + src={icon.src} + width={size} + height={size} + alt={icon.alt} + /> + ); }
591-592: Improve accessibility of detection IDs.The detection IDs are displayed with small typography that may be difficult to read. Consider using a tooltip or a more accessible way to display these identifiers.
-<Typography variant="caption">{det.id}</Typography> +<Tooltip title={`Detection ID: ${det.id}`}> + <Typography variant="caption" sx={{ cursor: 'pointer' }}>{det.id}</Typography> +</Tooltip>
47-56: Group related imports.Consider grouping related imports together for better readability. GraphQL imports could be organized by type (queries, mutations, fragments).
import { AudioCategory, BoutQuery, FeedQuery, +} from "@/graphql/generated"; +import { useCreateBoutMutation, useDetectionsQuery, useGetCurrentUserQuery, useListFeedStreamsQuery, useUpdateBoutMutation, } from "@/graphql/generated";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/src/graphql/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
ui/codegen.ts(1 hunks)ui/src/components/Bouts/BoutPage.tsx(1 hunks)ui/src/pages/bouts/new/[feedSlug].tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
ui/codegen.ts (1)
25-25: Good update to enum handling.Changing from
enumsAsTypestoenumsAsConstis a positive improvement. This configuration will generate TypeScript enums as const objects with string union types, providing better type safety and allowing use of enum values as discriminated unions in your components.ui/src/pages/bouts/new/[feedSlug].tsx (2)
12-12: Verify the hardcoded future date.The component uses a hardcoded date far in the future (2025). Is this intentional for development purposes, or should this be a dynamic value?
29-36: Add error handling for feed query failures.The component only handles loading and "not found" states but doesn't handle query errors.
Add error handling:
- if (!feedSlug || feedQueryResult.isLoading) return <LoadingSpinner mt={5} />; - if (!feed) return <p>Feed not found</p>; + if (!feedSlug || feedQueryResult.isLoading) return <LoadingSpinner mt={5} />; + if (feedQueryResult.error) return <p>Error loading feed: {feedQueryResult.error.message}</p>; + if (!feed) return <p>Feed not found</p>;ui/src/components/Bouts/BoutPage.tsx (2)
193-197: Enhance error handling in thesaveBoutfunction.The error handling could be improved by:
- Adding type safety for error codes
- Handling network errors
- Adding loading state management
+type BoutFormErrors = { + audioCategory?: string; + startTime?: string; + network?: string; +}; + const [boutForm, setBoutForm] = useState<{ - errors: Record<string, string>; + errors: BoutFormErrors; + isLoading: boolean; }>({ errors: {}, + isLoading: false, });
231-235: Replace setTimeout with useEffect for cleanup.Using
setTimeoutwithout cleanup can lead to memory leaks if the component unmounts before the timeout completes.Use
useEffectfor proper cleanup:- setBoutSaved(true); - setTimeout(() => { - setBoutSaved(false); - }, 5000); + setBoutSaved(true); + const timer = setTimeout(() => { + setBoutSaved(false); + }, 5000); + return () => clearTimeout(timer);
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
ui/src/components/Bouts/BoutPage.tsx (3)
193-197: 🛠️ Refactor suggestionAdd loading state to the form state.
The form state doesn't track loading status, which would improve user experience by disabling the save button while a save operation is in progress.
const [boutForm, setBoutForm] = useState<{ errors: Record<string, string>; + isLoading: boolean; }>({ errors: {}, + isLoading: false, });
231-235:⚠️ Potential issueReplace setTimeout with useEffect for proper cleanup.
Using
setTimeoutwithout cleanup can lead to memory leaks if the component unmounts before the timeout completes.- setBoutSaved(true); - setTimeout(() => { - setBoutSaved(false); - }, 5000); + setBoutSaved(true); + const timer = setTimeout(() => { + setBoutSaved(false); + }, 5000); + return () => clearTimeout(timer);This should be wrapped in a useEffect hook:
+ useEffect(() => { + if (boutSaved) { + const timer = setTimeout(() => { + setBoutSaved(false); + }, 5000); + return () => clearTimeout(timer); + } + }, [boutSaved]);
239-267: 🛠️ Refactor suggestionEnhance error handling and add loading state to saveBout function.
Improve the user experience by updating loading states during form submission and handling errors more robustly.
const saveBout = () => { setBoutForm((form) => ({ ...form, errors: {} })); if (audioCategory && boutStartTime) { + setBoutForm((form) => ({ ...form, isLoading: true })); if (isNew) { createBoutMutation.mutate({ feedId: feed.id, startTime: boutStartTime, endTime: boutEndTime, category: audioCategory, + }, { + onSettled: () => { + setBoutForm((form) => ({ ...form, isLoading: false })); + } }); } else if (bout) { updateBoutMutation.mutate({ id: bout.id, startTime: boutStartTime, endTime: boutEndTime, category: audioCategory, + }, { + onSettled: () => { + setBoutForm((form) => ({ ...form, isLoading: false })); + } }); } } else { const errors: Record<string, string> = {}; if (!audioCategory) { errors["audioCategory"] = "Audio category required"; } if (!boutStartTime) { errors["startTime"] = "Bout start time required"; } setBoutForm((form) => ({ ...form, errors })); } };ui/src/components/Bouts/SpectrogramTimeline.tsx (3)
39-74: 🛠️ Refactor suggestionAdd error handling for invalid time values in
centerWindow.The
centerWindowfunction should validate time values to prevent potential runtime errors.function centerWindow( spectrogramWindow: MutableRefObject<HTMLDivElement | null>, targetTime: Date, timelineStartTime: Date, pixelsPerMinute: number, setWindowStartTime: (value: SetStateAction<Date | undefined>) => void, setWindowEndTime: (value: SetStateAction<Date | undefined>) => void, playerControls?: PlayerControls, ) { + if (!(targetTime instanceof Date) || isNaN(targetTime.getTime())) { + console.error('Invalid targetTime provided to centerWindow'); + return; + } + + if (!(timelineStartTime instanceof Date) || isNaN(timelineStartTime.getTime())) { + console.error('Invalid timelineStartTime provided to centerWindow'); + return; + } + if (spectrogramWindow.current) { const offset = timeToOffset(targetTime, timelineStartTime, pixelsPerMinute); spectrogramWindow.current.scrollLeft = offset - spectrogramWindow.current.clientWidth / 2; if (spectrogramWindow.current) { setWindowStartTime( offsetToTime( spectrogramWindow.current.scrollLeft, timelineStartTime, pixelsPerMinute, ), ); setWindowEndTime( offsetToTime( spectrogramWindow.current.scrollLeft + spectrogramWindow.current.clientWidth, timelineStartTime, pixelsPerMinute, ), ); } if (playerControls?.setPlayerTime) { playerControls.setPlayerTime(targetTime); } } }
132-136:⚠️ Potential issueInclude dependencies in the
useCallbackhook for throttled functions.The ESLint warning is indicating that the
useCallbackhook is missing dependencies. The current implementation can cause stale closures.-// eslint-disable-next-line react-hooks/exhaustive-deps -const setWindowStartTime = useCallback( - _.throttle(setWindowStartTimeUnthrottled, 500, { trailing: false }), - [], -); +const setWindowStartTime = useCallback( + (value: SetStateAction<Date | undefined>) => { + const throttled = _.throttle( + (val: SetStateAction<Date | undefined>) => setWindowStartTimeUnthrottled(val), + 500, + { trailing: false } + ); + throttled(value); + }, + [setWindowStartTimeUnthrottled], +);Apply similar changes to the
setWindowEndTimefunction as well.
417-425:⚠️ Potential issueRemove the timelineEndTime prop from TimelineTickerLayer component.
The component receives a prop
timelineEndTimethat's not defined in its prop types, which will cause a TypeScript error.{windowStartTime && windowEndTime && ( <TimelineTickerLayer timelineStartTime={timelineStartTime} - timelineEndTime={timelineEndTime} windowStartTime={windowStartTime} windowEndTime={windowEndTime} pixelsPerMinute={pixelsPerMinute} zIndex={3} /> )}
🧹 Nitpick comments (4)
ui/src/components/Bouts/BoutPage.tsx (2)
297-304: Add disabled state to the save button based on loading state.The save button should reflect the loading state to provide user feedback when an operation is in progress.
<Button variant="contained" size={isDesktop ? "large" : "small"} onClick={saveBout} sx={{ whiteSpace: "nowrap" }} + disabled={boutForm.isLoading} > - {isNew ? "Create" : "Update"} bout + {boutForm.isLoading ? "Saving..." : (isNew ? "Create" : "Update") + " bout"} </Button>
635-670: Add a default return value to the CategoryIcon component.The
CategoryIconcomponent should return a default fallback icon for cases where an unsupported category is provided.function CategoryIcon({ audioCategory, size, }: { audioCategory: AudioCategory; size?: number; }) { size = size ?? 15; if (audioCategory === "BIOPHONY") return ( <Image src={whaleFlukeIconImage.src} width={size} height={size} alt="Whale fluke icon" /> ); if (audioCategory === "ANTHROPHONY") return ( <Image src={vesselIconImage.src} width={size} height={size} alt="Vessel icon" /> ); if (audioCategory === "GEOPHONY") return ( <Image src={wavesIconImage.src} width={size} height={size} alt="Waves icon" /> ); + // Return a default icon for unknown categories + return ( + <GraphicEq + sx={{ width: size, height: size }} + /> + ); }ui/src/components/Bouts/SpectrogramTimeline.tsx (2)
94-104: Add type check or assertion for Date objects in rangesOverlap.The
rangesOverlapfunction should verify that the provided date parameters are valid Date objects to prevent runtime errors.export function rangesOverlap( startTime1?: Date, endTime1?: Date, startTime2?: Date, endTime2?: Date, ) { - if (startTime1 && endTime1 && startTime2 && endTime2) { + if ( + startTime1 instanceof Date && !isNaN(startTime1.getTime()) && + endTime1 instanceof Date && !isNaN(endTime1.getTime()) && + startTime2 instanceof Date && !isNaN(startTime2.getTime()) && + endTime2 instanceof Date && !isNaN(endTime2.getTime()) + ) { return startTime1 <= endTime2 && endTime1 >= startTime2; } return false; }
247-282: Extract touch handling logic to a separate function for better readability.The touch handling code is quite complex and could be made more maintainable by extracting it to a separate function.
+const handleTouchDrag = ( + containerCursorX: number, + windowWidth: number +) => { + if (!spectrogramWindow.current) return; + + const move = containerCursorX - windowStartX.current; + const offset = windowScrollX.current - move; + + if (offset < 0 || offset + windowWidth > timelineWidth) return; + + spectrogramWindow.current.scrollLeft = offset; + const targetTime = offsetToTime( + offset + windowWidth / 2, + timelineStartTime, + pixelsPerMinute, + ); + setPlayerTime(targetTime); + setWindowStartTime( + offsetToTime(windowScrollX.current, timelineStartTime, pixelsPerMinute), + ); + setWindowEndTime( + offsetToTime( + windowScrollX.current + spectrogramWindow.current.offsetWidth, + timelineStartTime, + pixelsPerMinute, + ), + ); +}; const handleTouchMove = (e: React.TouchEvent) => { if (!isDragging || !spectrogramWindow.current) return; e.preventDefault(); const containerCursorX = e.touches[0].pageX - spectrogramWindow.current.offsetLeft; const windowWidth = spectrogramWindow.current.offsetWidth; - const move = containerCursorX - windowStartX.current; - const offset = windowScrollX.current - move; - const windowWidth = spectrogramWindow.current.offsetWidth; - if (offset < 0) return; - if (offset + windowWidth > timelineWidth) return; - spectrogramWindow.current.scrollLeft = offset; - const targetTime = offsetToTime( - offset + windowWidth / 2, - timelineStartTime, - pixelsPerMinute, - ); - setPlayerTime(targetTime); - setWindowStartTime( - offsetToTime(windowScrollX.current, timelineStartTime, pixelsPerMinute), - ); - setWindowEndTime( - offsetToTime( - windowScrollX.current + spectrogramWindow.current.offsetWidth, - timelineStartTime, - pixelsPerMinute, - ), - ); + handleTouchDrag(containerCursorX, windowWidth); };Apply a similar refactoring to the
handleMouseMovefunction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/src/components/Bouts/BoutPage.tsx(1 hunks)ui/src/components/Bouts/SpectrogramTimeline.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
ui/src/components/Bouts/BoutPage.tsx (1)
38-38: Remove or rename the unusedlogvariable.The import of lodash is used only for a throttled log function that's not being used anywhere in the component.
-import _ from "lodash"; +import { startCase, toLower } from "lodash";Then update lines 538 and 559 to use the imported functions:
- {_.startCase(_.toLower(category))} + {startCase(toLower(category))}- <Typography>{_.startCase(_.toLower(bout.category))}</Typography> + <Typography>{startCase(toLower(bout.category))}</Typography>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
server/lib/orcasite/radio/aws_client.ex (1)
51-51: Improvement to Lambda invocation reliability.Increasing the maximum retry attempts from 1 to 3 is a good enhancement that will improve the robustness of the spectrogram generation process when dealing with transient AWS service issues.
Consider also implementing a backoff strategy to complement the increased retry attempts. This would help prevent overwhelming the Lambda service during periods of instability.
|> ExAws.request( http_opts: [recv_timeout: :timer.minutes(2)], - retries: [max_attempts: 3] + retries: [max_attempts: 3, base_backoff_in_ms: 100, max_backoff_in_ms: 10_000] )ui/src/components/Bouts/BoutPage.tsx (2)
217-237: Use a cleanup pattern for the setTimeout.
When the component unmounts, the setTimeout callback may still run, attempting to update an unmounted component and leading to potential memory leaks. Consider migrating touseEffectwith a cleanup function to manage this timer.Example refactor:
} else { setBoutSaved(true); - setTimeout(() => { - setBoutSaved(false); - }, 5000); + const timerId = setTimeout(() => { + setBoutSaved(false); + }, 5000); + // using a cleanup effect + useEffect(() => { + return () => clearTimeout(timerId); + }, []); }
635-670: Refactor CategoryIcon using a switch statement or a mapping.
Using multipleifblocks is fine, but switching to a single switch statement or an object map may improve readability and make additions simpler.Example refactor:
-function CategoryIcon({ - audioCategory, - size, -}: { - audioCategory: AudioCategory; - size?: number; -}) { - size = size ?? 15; - if (audioCategory === "BIOPHONY") - return ( - <Image src={whaleFlukeIconImage.src} width={size} height={size} alt="Whale fluke icon" /> - ); - if (audioCategory === "ANTHROPHONY") - return ( - <Image src={vesselIconImage.src} width={size} height={size} alt="Vessel icon" /> - ); - if (audioCategory === "GEOPHONY") - return ( - <Image src={wavesIconImage.src} width={size} height={size} alt="Waves icon" /> - ); -} +function CategoryIcon({ audioCategory, size = 15 }: { audioCategory: AudioCategory; size?: number }) { + switch (audioCategory) { + case "BIOPHONY": + return <Image src={whaleFlukeIconImage.src} alt="Whale fluke icon" width={size} height={size} />; + case "ANTHROPHONY": + return <Image src={vesselIconImage.src} alt="Vessel icon" width={size} height={size} />; + case "GEOPHONY": + return <Image src={wavesIconImage.src} alt="Waves icon" width={size} height={size} />; + default: + return null; + } +}ui/src/components/Player/BoutPlayer.tsx (2)
128-128: Leverage optional chaining for cleaner, safer function calls.
Instead ofsetPlayerTimeRef && setPlayerTimeRef(time), use an optional chain for improved readability.- setPlayerTimeRef && setPlayerTimeRef(time); + setPlayerTimeRef?.(time);🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
236-237: Use optional chaining to handle undefined playerDateTime gracefully.
Ensures cleaner conditional rendering of the date without needing explicit checks.- {playerDateTime !== undefined && playerDateTime.toLocaleDateString()} + {playerDateTime?.toLocaleDateString()}🧰 Tools
🪛 Biome (1.9.4)
[error] 236-237: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
server/lib/orcasite/radio/audio_image.ex (2)
185-216: Status handling improved in generate_spectrogram action.The error handling flow now uses
:erroredinstead of:failed, which provides better status differentiation. Good use offorce_change_attributeto explicitly set the last_error to nil on success.Consider adding more structured error information rather than just using
inspect(error), which might make debugging easier in production.- |> Ash.Changeset.force_change_attribute(:last_error, inspect(error)) + |> Ash.Changeset.force_change_attribute(:last_error, format_error(error))You could add a private helper function:
defp format_error(error) do case error do %{message: message} -> message error when is_binary(error) -> error other -> inspect(other) end end
218-220: New set_failed action provides explicit status control.This explicit action to set status to
:failedcomplements the automatic transition to:erroredin the generate_spectrogram action.Consider adding documentation to clarify when to use
:failedvs:erroredstatus to ensure consistent usage across the application.update :set_failed do + description "Explicitly mark an audio image as failed (vs. errored which is used for processing errors)" change set_attribute(:status, :failed) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/lib/orcasite/radio/audio_image.ex(2 hunks)server/lib/orcasite/radio/aws_client.ex(1 hunks)server/lib/orcasite/radio/workers/generate_spectrogram.ex(2 hunks)ui/src/components/Bouts/BoutPage.tsx(1 hunks)ui/src/components/Player/BoutPlayer.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/lib/orcasite/radio/workers/generate_spectrogram.ex
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/components/Player/BoutPlayer.tsx
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 236-237: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
🔇 Additional comments (1)
server/lib/orcasite/radio/audio_image.ex (1)
225-229: GraphQL interface extended with feed filtering.The attribute type syntax is simplified and the new query is properly exposed through GraphQL. This ensures the new feed filtering functionality is accessible to clients.
ui/src/components/Bouts/BoutPage.tsx
Outdated
| const saveBout = () => { | ||
| setBoutForm((form) => ({ ...form, errors: {} })); | ||
| if (audioCategory && boutStartTime) { | ||
| if (isNew) { | ||
| createBoutMutation.mutate({ | ||
| feedId: feed.id, | ||
| startTime: boutStartTime, | ||
| endTime: boutEndTime, | ||
| category: audioCategory, | ||
| }); | ||
| } else if (bout) { | ||
| updateBoutMutation.mutate({ | ||
| id: bout.id, | ||
| startTime: boutStartTime, | ||
| endTime: boutEndTime, | ||
| category: audioCategory, | ||
| }); | ||
| } | ||
| } else { | ||
| const errors: Record<string, string> = {}; | ||
| if (!audioCategory) { | ||
| errors["audioCategory"] = "Audio category required"; | ||
| } | ||
| if (!boutStartTime) { | ||
| errors["startTime"] = "Bout start time required"; | ||
| } | ||
| setBoutForm((form) => ({ ...form, errors })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding loading state management and preventing multiple submissions.
Currently, the UI does not show any loading indicator and allows multiple rapid clicks on “Create/Update bout.” Implement a loading state to disable the button while awaiting server response, improving user feedback and preventing duplicate requests.
| const intervalRef = useRef<NodeJS.Timeout>(); | ||
|
|
There was a problem hiding this comment.
Clear the interval on component unmount to avoid memory leaks.
Currently, the interval is cleared when the player pauses, but not when the component unmounts. Add a cleanup effect that clears any ongoing interval, ensuring no background tasks remain on unmount.
Example fix:
useEffect(() => {
return () => {
clearInterval(intervalRef.current);
};
}, []);Also applies to: 133-153
… state when creating a bout, remove redundant gql fields
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ui/src/components/Bouts/BoutPage.tsx (1)
235-239: Consider replacing the setTimeout approach with a useEffect cleanup.Currently, the
setTimeoutcall to resetboutSavedmight remain active if the user navigates away quickly. Using auseEffecthook and clearing the timer on unmount would help prevent potential memory leaks:else { setBoutSaved(true); - setTimeout(() => { - setBoutSaved(false); - }, 5000); + const timer = setTimeout(() => setBoutSaved(false), 5000); + return () => clearTimeout(timer); }ui/src/components/Player/BoutPlayer.tsx (2)
129-135: Adopt optional chaining for cleaner code.Instead of
setPlayerTimeRef && setPlayerTimeRef(time);, consider using optional chaining for readability:- setPlayerTimeRef && setPlayerTimeRef(time); + setPlayerTimeRef?.(time);🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
241-242: Use optional chaining forplayerDateTimechecks.Short-circuiting with
!== undefinedcan be replaced with an optional chain for succinctness and clarity:- {playerDateTime !== undefined && playerDateTime.toLocaleDateString()} + {playerDateTime?.toLocaleDateString()}🧰 Tools
🪛 Biome (1.9.4)
[error] 241-242: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/Bouts/SpectrogramTimeline.tsx (1)
221-246: Consider reducing the 100ms interval inuseEffectfor performance.Re-centering the timeline every 100ms, especially on slower devices or large timelines, might be intensive. Implementing a more optimal throttling strategy or increasing the interval could improve performance and reduce CPU usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ui/src/components/Bouts/BaseAudioWidthLayer.tsx(1 hunks)ui/src/components/Bouts/BoutPage.tsx(1 hunks)ui/src/components/Bouts/SpectrogramTimeline.tsx(1 hunks)ui/src/components/Player/BoutPlayer.tsx(1 hunks)ui/src/graphql/fragments/BoutParts.graphql(1 hunks)ui/src/pages/bouts/[boutId].tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/src/graphql/fragments/BoutParts.graphql
- ui/src/pages/bouts/[boutId].tsx
- ui/src/components/Bouts/BaseAudioWidthLayer.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/components/Player/BoutPlayer.tsx
[error] 133-133: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 241-242: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
ui/src/components/Bouts/BoutPage.tsx (1)
243-272: Nice unified error handling and loading state logic.Your
saveBoutfunction correctly assigns and unassignsisSaving, updates form errors, and handles successful creation or update. This is a clean approach to error handling in React.ui/src/components/Bouts/SpectrogramTimeline.tsx (1)
185-191: MovespectrogramControlsassignment into auseEffecthook.Directly assigning
spectrogramControls.currentin the component body can cause lifecycle issues. As previously noted, consider wrapping it in auseEffectfor safer updates.- if (spectrogramControls) { - spectrogramControls.current = { - goToTime, - zoomIn: () => setZoomLevel((zoom) => _.clamp(zoom * 2, minZoom, maxZoom)), - zoomOut: () => setZoomLevel((zoom) => _.clamp(zoom / 2, minZoom, maxZoom)), - }; - } + useEffect(() => { + if (spectrogramControls) { + spectrogramControls.current = { + goToTime, + zoomIn: () => setZoomLevel((zoom) => _.clamp(zoom * 2, minZoom, maxZoom)), + zoomOut: () => setZoomLevel((zoom) => _.clamp(zoom / 2, minZoom, maxZoom)), + }; + } + }, [spectrogramControls, goToTime, minZoom, maxZoom]);
* first commit * test commit * toolpad core with error * moderator ui dashboard layout with known nonbreaking TS errors * moderator UI draft PR for Feb hackathon * reverting file that didn't need to change * backup before merging new bout * pushing to skander/new-bout instead of main
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (39)
ui/src/graphql/queries/list2Feeds.graphql (2)
2-17: Consider Fragment Reuse for MaintainabilityThe field selection within the
feedsquery is comprehensive and explicit. For consistency and ease of future modifications, consider reusing an existing fragment (e.g.,FeedParts) if it already encapsulates these feed fields. This approach reduces code duplication and improves maintainability across similar queries.
18-18: Extraneous Trailing LineThere is a lone trailing line at the end of the file. While not causing any issues, removing unintentional whitespace can keep the file clean.
ui/src/graphql/queries/listDetections2.graphql (1)
1-6: Ensure Proper Variable Type Constraints.The query parameters for
detections2are clearly defined and easy to follow. One suggestion is to consider whether any of the variables (for example,$limit) should be non-null (usingInt!) if they are required to produce a valid result. This can help prevent runtime errors or unexpected behavior if a variable is unintentionally omitted.ui/src/pages/moderator/bouts.tsx (1)
5-5: Consider renaming for consistency.The component is named
ModeratorLearnPagebut is in a file namedbouts.tsx. Consider renaming the component toModeratorBoutsPageto match the file name, or rename the file tolearn.tsxto match the component name.ui/src/pages/moderator/reports.tsx (1)
3-5: Naming inconsistency between import and file path.The component is imported as
DetectionsPagefrom@/pages/reports. This naming inconsistency might be confusing. Consider either:
- Renaming the imported component to match the file path (
ReportsPage)- Updating the import path to match the component name (
@/pages/detections)ui/src/pages/moderator/index.tsx (2)
6-6: Consider renaming for clarity.The component is named
NewFeedsPagebut renders aCandidatescomponent. This naming might be confusing. Consider either:
- Renaming the component to
ModeratorCandidatesPageto match what it renders- Ensuring that the
Candidatescomponent is indeed related to new feeds
7-11: Unnecessary fragment wrapper.Since there's only one child component, the fragment wrapper (
<></>) is unnecessary. You can simplify this to:- return ( - <> - <Candidates /> - </> - ); + return <Candidates />;ui/src/pages/moderator/json.tsx (1)
6-18: Add error handling and improve JSON display.The page correctly fetches and displays JSON data, but could benefit from:
- Error handling for the GraphQL query
- Improved styling or structure for better readability
- Possibly adding a toggle to show/hide each JSON section
const JSONPage: NextPageWithLayout = () => { const { combined } = useData(); // get hydrophone feed list const feedsQueryResult = useFeedsQuery(); + const loading = feedsQueryResult.loading; + const error = feedsQueryResult.error; const feeds = feedsQueryResult.data?.feeds ?? []; return ( <> + {error && <div className="text-red-500">Error loading feeds: {error.message}</div>} + {loading && <div>Loading...</div>} + <h2 className="text-xl font-bold mb-2">Combined Data:</h2> <pre className="bg-gray-100 p-4 rounded mb-4 overflow-auto max-h-96">{JSON.stringify(combined, null, 2)}</pre> + <h2 className="text-xl font-bold mb-2">Feeds Data:</h2> <pre className="bg-gray-100 p-4 rounded overflow-auto max-h-96">{JSON.stringify(feeds, null, 2)}</pre> </> ); };ui/src/components/layouts/ToolpadLayout.tsx (1)
22-24: Consider updating the branding titleThe branding title "My Toolpad Core App" appears to be a placeholder. Consider updating it to match your application's actual name for consistency across the UI.
const BRANDING = { - title: "My Toolpad Core App", + title: "Application Name", // Replace with your actual application name };ui/src/components/FeedList.tsx (1)
18-18: Add loading and error statesThe component currently returns null if sortedFeeds is falsy, which happens during loading or on error. Consider adding explicit loading and error states for a better user experience.
- if (!sortedFeeds) return null; + if (feedsQueryResult.isLoading) { + return ( + <Container maxWidth="sm"> + <Typography variant="body1">Loading feeds...</Typography> + </Container> + ); + } + + if (feedsQueryResult.isError) { + return ( + <Container maxWidth="sm"> + <Typography variant="body1" color="error"> + Error loading feeds. Please try again. + </Typography> + </Container> + ); + } + + if (!sortedFeeds?.length) { + return ( + <Container maxWidth="sm"> + <Typography variant="body1">No feeds available.</Typography> + </Container> + ); + }ui/src/pages/moderator/hydrophones.tsx (1)
5-7: Consider more descriptive naming or extended logic.
This component is namedhydrophones.tsx, yet it simply reusesFeedsPagefrom/listen. If distinct functionality or data is intended for hydrophones, consider adding a unique implementation or renaming the component/file for clarity.ui/src/pages/moderator/playertest.tsx (2)
22-22: Use optional chaining for clarity.
Instead of referencingcandidate && candidate.detections, consider simplifying to:- const detections = candidate && candidate.detections; + const detections = candidate?.detections;🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
82-88: Combine multiple setState calls to reduce extra re-renders.
You currently callsetPlayerPropstwice in the same effect, which may cause additional re-render cycles. Combine them into a single call if feasible:setPlayerProps({ feed: feed ? feed : feedsData[0], timestamp: startTimestamp, startOffset: startOffset, endOffset: endOffset, audioUri: lastDetection.audioUri || "", });ui/src/components/ChartSelect.tsx (2)
31-31: Use a more descriptive "id" or remove it if not needed.
"id='demo-simple-select'" might be a leftover from an example. Rename or remove it for clarity and maintainability.- id="demo-simple-select" + id="chart-select-dropdown"
7-13: Consider adding tests for this UI component.
This component is critical for user interaction. You could enhance reliability by adding unit tests that validate the dropdown list rendering and theonChangeevent behavior.Also applies to: 15-45
ui/src/components/PlayBar.tsx (2)
82-87: Avoid multiple setState calls inside the same effect.
Currently, callingsetPlayerPropsmultiple times can cause extra re-renders. For better performance, unify your logic into a single call:setPlayerProps({ feed: feed ? feed : feedsData[0], timestamp: startTimestamp, startOffset, endOffset, audioUri: lastDetection?.audioUri || "" });
225-225: Use optional chaining in onClick handlers.
ReplaceonClick={() => item.onClick && item.onClick()}withonClick={() => item.onClick?.()}to simplify logic and improve readability.-onClick={() => item.onClick && item.onClick()} +onClick={() => item.onClick?.()}-onClick={() => page.onClick && page.onClick()} +onClick={() => page.onClick?.()}Also applies to: 282-282
🧰 Tools
🪛 Biome (1.9.4)
[error] 225-225: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/types/DataTypes.ts (3)
6-12: Consider restricting thetypefield to defined options.Right now,
type: stringin theHumanDatainterface is quite broad. You could use a string literal union (e.g.,"human" | "ai") or an enum to reduce the risk of typos or invalid assignments.
9-9: Unify optional comments into a single optional property.Instead of
comments: string | null | undefined;, consider using a single optional property likecomments?: stringto simplify usage.- comments: string | null | undefined; + comments?: string;
37-44: Optional clarifications for dataset structure.The
Datasetinterface includes separate arrays forhuman,ai, andcombined. Ensure thatcombineddoesn’t become stale if future code directly updateshumanorai. Ideally, define a single source of truth or always reconstructcombinedas needed.ui/src/pages/moderator/[candidateId].tsx (3)
60-69: Combine repeated conditions.Lines 63–66 and 66–69 check the same condition twice. You could unify them, improving readability:
- if (time >= startTime && time <= endTime) { - console.log("both true"); - } - if (time >= startTime && time <= endTime) { - arr.push(d); - } + if (time >= startTime && time <= endTime) { + console.log("both true"); // if you really need this log + arr.push(d); + }
35-35: Remove or replace console.log statements in production code.Debugging logs like
console.log("startTime:...")or"both true"spam logs in production. Consider a proper logging mechanism or remove them before deployment.Also applies to: 64-64, 65-65, 75-75, 76-76
82-86: Check for undefined array elements before accessing them.
detections?.human[0]?.feedIdmay still cause issues ifdetections.humanis empty. Wrap indexing in safety checks or use optional chaining consistently to avoid runtime errors.- const feed = feeds.filter((f) => f.id === detections?.human[0]?.feedId)[0]; + const feed = feeds.find((f) => f.id === detections?.human[0]?.feedId) ?? null;ui/src/components/layouts/DrawerLayout.tsx (1)
128-131: Use optional chaining for clarity.In the
navDivfunction, you can simplify thediv.children && div.children.map(...)pattern with optional chaining, e.g.div.children?.map(...).- {div.children && - div.children.map((item) => - listItem(item.title, item.path, item.icon), - )} + {div.children?.map((item) => + listItem(item.title, item.path, item.icon) + )}🧰 Tools
🪛 Biome (1.9.4)
[error] 128-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/layouts/ModeratorLayout.tsx (2)
101-114: Remove unreachable code after returns instandardizeFeedName.The
break;statements are unreachable because thereturnstatement will exit the function. Removing them simplifies control flow.case "Beach Camp at Sunset Bay": return "Sunset Bay"; - break; ... case "North SJC": return "North San Juan Channel"; - break; ... default: return name; - break;🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 107-107: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 110-110: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 113-113: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
258-261: Leverage optional chaining for safer array mapping.You can replace
div.children && div.children.map(...)withdiv.children?.map(...)to simplify the code and prevent potential runtime errors ifchildrenis undefined.🧰 Tools
🪛 Biome (1.9.4)
[error] 258-261: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/Player/CandidateCardAIPlayer.tsx (1)
97-123: Use optional chaining for cleaner callback invocation.
Wherever you check a callback before calling, you can simplify the code with optional chaining:- onPlayerInit && onPlayerInit(player); + onPlayerInit?.(player); - onPlay && onPlay(); + onPlay?.(); - onPlayerEnd && onPlayerEnd(); + onPlayerEnd?.();🧰 Tools
🪛 Biome (1.9.4)
[error] 97-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 106-106: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/Player/CandidateCardPlayer.tsx (3)
87-123: Use optional chaining for callback props.The
handleReadyfunction has proper event handling for the video player, but there are places where optional chaining could be used for callback props to improve code safety.According to static analysis, consider using optional chaining on these lines:
- onPlayerInit && onPlayerInit(player); + onPlayerInit?.(player); - onPlay && onPlay(); + onPlay?.(); - onPlayerEnd && onPlayerEnd(); + onPlayerEnd?.();🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 97-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
154-161: Consider separating development logging from cleanup effects.The useEffect hook combines two unrelated operations: development logging and cleanup. It would be cleaner to separate these concerns.
useEffect(() => { if (process.env.NODE_ENV === "development" && hlsURI) { console.log(`New stream instance: ${hlsURI}`); } + }, [hlsURI]); + + useEffect(() => { return () => { setPlayerStatus("idle"); }; - }, [hlsURI, feed.nodeName]); + }, []);
243-249: Time formatting utility could be more reusable.The time formatting utility is well-implemented but could be moved to a common utilities file for reuse across the application.
Consider extracting the
formattedSecondsfunction to a shared utilities file so it can be reused by other components that need to format time values.ui/src/pages/moderator/candidates.tsx (9)
13-72: Consider using enums or constants for selection options.The time range, increment, and category selections are defined as arrays of objects with labels and values. Consider using TypeScript enums or separate constant objects to better organize these configuration values.
+// Define enum or type for time ranges +enum TimeRange { + SEVEN_DAYS = 7 * 24 * 60 * 60 * 1000, + THREE_DAYS = 3 * 24 * 60 * 60 * 1000, + ONE_DAY = 24 * 60 * 60 * 1000 +} + +// Use the enum values in your select options const timeRangeSelect = [ { label: "Last 7 days", - value: sevenDays, + value: TimeRange.SEVEN_DAYS, }, // ... ];
84-142: Consider performance optimization increateCandidatesfunction.The
createCandidatesfunction has nested loops and could potentially be optimized for better performance.The
findLastMatchingArrayfunction is called for each element, which could be inefficient for large datasets. Consider using a map to keep track of the last array for each hydrophone:const createCandidates = ( dataset: CombinedData[], interval: number, ): Candidate[] => { const candidates: Array<Array<CombinedData>> = []; + const lastArrayByHydrophone = new Map<string, Array<CombinedData>>(); const sort = dataset.sort( (a, b) => Date.parse(b.timestampString) - Date.parse(a.timestampString), ); sort.forEach((el: CombinedData) => { if (!candidates.length) { const firstArray = []; firstArray.push(el); candidates.push(firstArray); + lastArrayByHydrophone.set(el.hydrophone, firstArray); } else { const hydrophone = el.hydrophone; - const findLastMatchingArray = () => { - for (let i = candidates.length - 1; i >= 0; i--) { - if (candidates[i][0].hydrophone === hydrophone) { - return candidates[i]; - } - } - }; - const lastMatchingArray = findLastMatchingArray(); + const lastMatchingArray = lastArrayByHydrophone.get(hydrophone); const lastTimestamp = lastMatchingArray && lastMatchingArray[lastMatchingArray.length - 1].timestampString; // ... rest of the code🧰 Tools
🪛 Biome (1.9.4)
[error] 108-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
108-109: Use optional chaining for safer code.The static analysis tool correctly identified a place where optional chaining would be safer.
- const lastTimestamp = - lastMatchingArray && - lastMatchingArray[lastMatchingArray.length - 1].timestampString; + const lastTimestamp = lastMatchingArray?.[lastMatchingArray.length - 1].timestampString;🧰 Tools
🪛 Biome (1.9.4)
[error] 108-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
182-193: Remove commented-out code.There's a block of commented-out code that doesn't serve any documentation purpose. This should be removed for cleaner code.
- // const [playing, setPlaying] = useState({ - // index: -1, - // status: "ready", - // }); - - // const changeListState = (index: number, status: string) => { - // setPlaying((prevState) => ({ - // ...prevState, - // index: index, - // status: status, - // })); - // };
197-197: Consider typing theplayersref.The
playersref is initialized with an empty object without a type. Consider adding proper typing.- const players = useRef({}); + const players = useRef<Record<string, any>>({});Or preferably, create a more specific interface for what's stored in the players object.
205-218: Consider extracting filter logic to a separate function.The filtering logic is embedded in the component, making it harder to read and test. Consider extracting it to a separate function.
+ const filterData = (data: CombinedData[], filters: typeof filters) => { + return data.filter((el: CombinedData) => { + return ( + (filters.hydrophone === "All hydrophones" || + el.hydrophone === filters.hydrophone) && + (filters.category === "All categories" || + el.newCategory.toLowerCase() === filters.category) + ); + }); + }; - const filteredData = combined.filter((el: CombinedData) => { - return ( - // uncomment this to block Orcahello data - // el.type === "human" && - - // Disabling timerange filter for now because seed data is all from 2023 - // (Date.parse(el.timestamp) >= min) && - - (filters.hydrophone === "All hydrophones" || - el.hydrophone === filters.hydrophone) && - (filters.category === "All categories" || - el.newCategory.toLowerCase() === filters.category) - ); - }); + const filteredData = filterData(combined, filters);
224-240: Sorting functions could be optimized.The sorting functions
sortDescendingandsortAscendingare almost identical except for the order of operands. Consider creating a single sort function with a direction parameter.- const sortDescending = (array: Candidate[]) => { - const sort = array.sort( - (a, b) => - handledGetTime(b.array[0].timestamp) - - handledGetTime(a.array[0].timestamp), - ); - return sort; - }; - - const sortAscending = (array: Candidate[]) => { - const sort = array.sort( - (a, b) => - handledGetTime(a.array[0].timestamp) - - handledGetTime(b.array[0].timestamp), - ); - return sort; - }; + const sortCandidates = (array: Candidate[], direction: 'asc' | 'desc') => { + return [...array].sort((a, b) => { + const timeA = handledGetTime(a.array[0].timestamp); + const timeB = handledGetTime(b.array[0].timestamp); + return direction === 'asc' ? timeA - timeB : timeB - timeA; + }); + };Note that I've also changed to copying the array before sorting, as sorting in JavaScript mutates the array.
255-260: Simplify useEffect.The
useEffecthas a redundant statement - it setssortedCandidatesto[...candidates]twice.useEffect(() => { - setSortedCandidates((v) => [...candidates]); if (isSuccess) { setSortedCandidates((v) => [...candidates]); } }, [isSuccess]);
278-290: Duplicate component rendering logic.There's duplicate code for rendering
CandidateCardcomponents incandidateCardsandsortedCandidateCards. Consider extracting a function to avoid duplication.+ const renderCandidateCard = (candidate: Candidate, index: number) => ( + <CandidateCard + candidate={candidate} + key={index} + index={index} + players={players} + playNext={playNext} + /> + ); - const candidateCards = candidates.map( - (candidate: Candidate, index: number) => ( - <CandidateCard - candidate={candidate} - key={index} - index={index} - players={players} - playNext={playNext} - /> - ), - ); + const candidateCards = candidates.map(renderCandidateCard); - const sortedCandidateCards = sortedCandidates.map( - (candidate: Candidate, index: number) => ( - <CandidateCard - candidate={candidate} - key={index} - index={index} - players={players} - playNext={playNext} - /> - ), - ); + const sortedCandidateCards = sortedCandidates.map(renderCandidateCard);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.DS_Storeis excluded by!**/.DS_Storepackage-lock.jsonis excluded by!**/package-lock.jsonui/package-lock.jsonis excluded by!**/package-lock.jsonui/src/graphql/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (28)
package.json(1 hunks)server/priv/repo/seeds.exs(1 hunks)ui/package.json(2 hunks)ui/src/components/CandidateCard.tsx(1 hunks)ui/src/components/ChartSelect.tsx(1 hunks)ui/src/components/FeedList.tsx(1 hunks)ui/src/components/Header.tsx(1 hunks)ui/src/components/PlayBar.tsx(1 hunks)ui/src/components/Player/CandidateCardAIPlayer.tsx(1 hunks)ui/src/components/Player/CandidateCardPlayer.tsx(1 hunks)ui/src/components/ReportsBarChart.tsx(1 hunks)ui/src/components/layouts/DrawerLayout.tsx(1 hunks)ui/src/components/layouts/ModeratorLayout.tsx(1 hunks)ui/src/components/layouts/ToolpadLayout.tsx(1 hunks)ui/src/context/DataContext.tsx(1 hunks)ui/src/graphql/queries/list2Feeds.graphql(1 hunks)ui/src/graphql/queries/listDetections2.graphql(1 hunks)ui/src/pages/_app.tsx(1 hunks)ui/src/pages/moderator/[candidateId].tsx(1 hunks)ui/src/pages/moderator/bouts.tsx(1 hunks)ui/src/pages/moderator/candidates.tsx(1 hunks)ui/src/pages/moderator/hydrophones.tsx(1 hunks)ui/src/pages/moderator/index.tsx(1 hunks)ui/src/pages/moderator/json.tsx(1 hunks)ui/src/pages/moderator/learn.tsx(1 hunks)ui/src/pages/moderator/playertest.tsx(1 hunks)ui/src/pages/moderator/reports.tsx(1 hunks)ui/src/types/DataTypes.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- ui/src/pages/_app.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/pages/moderator/playertest.tsx
[error] 22-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/PlayBar.tsx
[error] 225-225: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 282-282: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/layouts/DrawerLayout.tsx
[error] 128-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/Player/CandidateCardAIPlayer.tsx
[error] 97-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 106-106: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/Player/CandidateCardPlayer.tsx
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 97-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/components/layouts/ModeratorLayout.tsx
[error] 104-104: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 107-107: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 110-110: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 113-113: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 258-261: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
ui/src/pages/moderator/candidates.tsx
[error] 108-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
🔇 Additional comments (31)
ui/src/graphql/queries/list2Feeds.graphql (1)
1-1: GraphQL Query Declaration is CorrectThe query declaration for
feeds2correctly defines an optional parameter$sortof type[FeedSortInput]. The overall syntax adheres to GraphQL standards.ui/src/graphql/queries/listDetections2.graphql (1)
7-24: Verify Field Selections Against Schema.The query correctly retrieves
count,hasNextPage, and the detailed list ofresultswith nested objects (feedandcandidate). Please verify that these field names and their structure exactly match the GraphQL schema definitions on the server side. Also, if these fields are consistently used across different parts of the application, consider extracting them into a GraphQL fragment (e.g., the newAudioImagePartsfragment) for improved reusability and maintainability.server/priv/repo/seeds.exs (2)
16-23: Feed activation and bucket standardization looks goodYou've activated the "Orcasound Lab (Haro Strait)" feed by uncommenting it and updated its bucket value to "audio-orcasound-net". This is consistent with the other bucket changes and aligns with what appears to be a standardization effort.
29-29: Consistent bucket naming standardization across all feedsGood job standardizing all bucket values to "audio-orcasound-net" (including the commented-out feeds). This consistency will make maintenance easier and ensures all feeds use the same storage location when active.
Also applies to: 37-37, 45-45, 53-53, 61-61
ui/src/components/Header.tsx (1)
38-39: Good UX improvement with the sticky header!Changing from static to sticky positioning allows the header to remain visible as users scroll down the page, which is especially helpful for navigating the new bout page. This ensures consistent access to navigation elements throughout the user experience.
ui/src/pages/moderator/bouts.tsx (1)
1-11: Component looks good!The implementation correctly wraps the BoutsPage component with the moderator layout. This follows the pattern of reusing common functionality with different layouts for different user roles.
ui/src/pages/moderator/reports.tsx (1)
1-11: Implementation is clean and consistent.The page correctly wraps the DetectionsPage component with the moderator layout, following the pattern established in other moderator pages.
ui/src/pages/moderator/index.tsx (1)
6-12: Component structure looks good.The implementation follows the established pattern for moderator pages.
ui/src/pages/moderator/learn.tsx (1)
1-11: Clean implementation of a moderator-specific learn pageThis looks good! The implementation follows Next.js patterns for creating pages with custom layouts. The ModeratorLearnPage component properly wraps the LearnPage with a moderator-specific layout using the getLayout pattern.
ui/src/components/layouts/ToolpadLayout.tsx (2)
10-20: Navigation structure is minimalThe current navigation only includes a Dashboard item. This may be intentional for now, but ensure it aligns with the planned functionality of the Toolpad interface. Consider whether additional navigation items will be needed.
26-34: Well-structured layout componentThe ToolpadLayout component is cleanly implemented with appropriate wrapping of children in the required providers and containers.
ui/src/context/DataContext.tsx (1)
1-23: Clean implementation of the DataContextThe DataContext implementation follows React best practices for context creation and consumption. The default values, hook, and provider are all properly structured.
ui/src/components/FeedList.tsx (2)
11-16: Appropriate use of useMemo for sortingGood use of useMemo to optimize the sorting operation, which will only recalculate when the data changes.
37-50: Proper implementation of getStaticPropsThe getStaticProps function correctly prefetches data for server-side rendering and returns the dehydrated state as props.
ui/src/pages/moderator/hydrophones.tsx (1)
9-11: Looks good.
The assignment ofModeratorFeedsPage.getLayout = getModeratorLayout;is consistent with Next.js custom layout usage. No issues found in the default export.ui/src/pages/moderator/playertest.tsx (1)
33-41: Verify empty array handling.
Be aware thatMath.min(...detections.map(/*...*/))andMath.max(...detections.map(/*...*/))will produceInfinityor-Infinityifdetectionsis an empty array. Verify that you handle the case of zero-length arrays to avoid unexpected behavior.ui/src/components/PlayBar.tsx (1)
49-88: Guard against empty arrays when calling Math.min / Math.max.
candidate.arraymight be empty. Passing an empty array toMath.min(...[])orMath.max(...[])results inInfinityor-Infinity. Ensure you handle this scenario gracefully.ui/src/types/DataTypes.ts (1)
35-35: Validate overlapping fields in multiple inheritance.
CombinedDataextends bothHumanDataandAIData. If they share any properties with different types, it can lead to conflicts. Verify that fields, such astimestampStringortype, don’t conflict across these interfaces.ui/src/pages/moderator/[candidateId].tsx (2)
31-36: Validate the splitting logic forcandidateId.Using
split("_")oncandidateIdassumes a consistent format ("start_end"). Ensure you gracefully handle unexpected or invalid tokens in real-world scenarios to avoid runtime errors.
172-186: Revisit fallback logic for selecting a player.Currently, you’re checking if
detections.human.lengththen show one player, otherwise checkdetections.ai.length, else show “no player found.” If both exist, only the human player is shown. Confirm this is the intended priority.ui/src/components/CandidateCard.tsx (1)
26-174: Looks good!
The logic for conditionally rendering different players and linking candidate timestamps is clear and straightforward.ui/src/components/Player/CandidateCardPlayer.tsx (7)
1-16: Dependencies and imports look appropriate.The imports are well-organized, with external libraries first, followed by internal modules. The dynamic import of VideoJS is a good optimization for speeding up initial page load.
21-49: Well-structured component with clear and typed props.The component has well-defined TypeScript interfaces for its props, making it clear what data it expects. The use of
Picktype for thefeedprop is a good practice to specify exactly which fields are needed.
50-57: State management is well-organized.The component properly initializes its state variables with appropriate types. Using
useReffor storing the player instance is the correct approach for maintaining references to imperative objects.
58-85: Memoize HLS URI configuration properly, but consider handling potential edge cases.The
useMemohook correctly prevents unnecessary recalculations of player options. Using a dummy URI for error triggering is a clever approach.Verify that the 404 path is properly handled in the error state. Make sure that your error handling logic in the component properly handles scenarios where the video source isn't available.
125-152: Error handling is comprehensive.The play/pause handler has good error handling, properly checking for error states and handling the AbortError exception. The overall approach is robust.
163-180: Slider handlers are well-implemented.The slider change handlers properly manage player state during seek operations. The use of optional chaining on
playerRef?.currentis good practice.
182-240: Well-structured UI layout with responsive considerations.The component has a clean, responsive layout with proper styling for both desktop and mobile views. The use of Material-UI's Box component with theme-based styling is excellent.
ui/src/pages/moderator/candidates.tsx (3)
74-82: Interface definition looks good.The Candidate interface is well-defined with clear properties for the data structure.
263-275: Consider using unique keys for CandidateCard components.Using array indices as React keys can lead to unexpected behavior if the array items get reordered. Consider using a more stable unique identifier if available.
<CandidateCard candidate={candidate} key={index} // Using index as key might cause issues if array items get reordered index={index} // ... />Is there any unique identifier in the
candidateobject that could be used as key instead of the array index?
293-368: Overall component structure is well organized.The component has a clear structure with filtering controls, sorting buttons, and a display of the filtered results. The Material-UI components are used consistently.
| if (dataset[i].newCategory.toLowerCase() === "whale") { | ||
| chartItem.whale += 1; | ||
| } | ||
| switch (dataset[i].newCategory.toLowerCase()) { | ||
| case "whale": | ||
| chartItem.whale += 1; | ||
| break; |
There was a problem hiding this comment.
Remove duplicated increment for "whale" category.
Lines 94-96 and 98-100 both increment chartItem.whale, which will double-count whale detections. Remove the redundant conditional to fix the overcounting.
- if (dataset[i].newCategory.toLowerCase() === "whale") {
- chartItem.whale += 1;
- }📝 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.
| if (dataset[i].newCategory.toLowerCase() === "whale") { | |
| chartItem.whale += 1; | |
| } | |
| switch (dataset[i].newCategory.toLowerCase()) { | |
| case "whale": | |
| chartItem.whale += 1; | |
| break; | |
| switch (dataset[i].newCategory.toLowerCase()) { | |
| case "whale": | |
| chartItem.whale += 1; | |
| break; |
| export default function Candidates() { | ||
| // replace this with a direct react-query... | ||
| const { | ||
| combined, | ||
| isSuccess, | ||
| }: { combined: CombinedData[] | undefined; isSuccess: boolean } = useData(); // this uses a context provider to call data once and make it available to all children -- this may not be better than just using the query hooks, kind of does the same thing | ||
|
|
||
| // get hydrophone feed list | ||
| const feedsQueryResult = useFeedsQuery(); | ||
| const feeds = feedsQueryResult.data?.feeds ?? []; | ||
|
|
||
| const [filters, setFilters] = useState({ | ||
| timeRange: threeDays, | ||
| timeIncrement: 15, | ||
| hydrophone: "All hydrophones", | ||
| category: "All categories", | ||
| }); | ||
|
|
||
| const [timeRange, setTimeRange] = useState(threeDays); | ||
| const [timeIncrement, setTimeIncrement] = useState(15); | ||
| const [hydrophone, setHydrophone] = useState("All hydrophones"); | ||
| const [category, setCategory] = useState("All categories"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unnecessary duplication of state.
There are duplicate state variables: filters and individual state variables for each filter property. This duplication can lead to synchronization issues.
Remove the individual state variables (timeRange, timeIncrement, etc.) and just use the filters object:
const [filters, setFilters] = useState({
timeRange: threeDays,
timeIncrement: 15,
hydrophone: "All hydrophones",
category: "All categories",
});
- const [timeRange, setTimeRange] = useState(threeDays);
- const [timeIncrement, setTimeIncrement] = useState(15);
- const [hydrophone, setHydrophone] = useState("All hydrophones");
- const [category, setCategory] = useState("All categories");Then update any functions that use these individual variables to use filters instead.
📝 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.
| export default function Candidates() { | |
| // replace this with a direct react-query... | |
| const { | |
| combined, | |
| isSuccess, | |
| }: { combined: CombinedData[] | undefined; isSuccess: boolean } = useData(); // this uses a context provider to call data once and make it available to all children -- this may not be better than just using the query hooks, kind of does the same thing | |
| // get hydrophone feed list | |
| const feedsQueryResult = useFeedsQuery(); | |
| const feeds = feedsQueryResult.data?.feeds ?? []; | |
| const [filters, setFilters] = useState({ | |
| timeRange: threeDays, | |
| timeIncrement: 15, | |
| hydrophone: "All hydrophones", | |
| category: "All categories", | |
| }); | |
| const [timeRange, setTimeRange] = useState(threeDays); | |
| const [timeIncrement, setTimeIncrement] = useState(15); | |
| const [hydrophone, setHydrophone] = useState("All hydrophones"); | |
| const [category, setCategory] = useState("All categories"); | |
| export default function Candidates() { | |
| // replace this with a direct react-query... | |
| const { | |
| combined, | |
| isSuccess, | |
| }: { combined: CombinedData[] | undefined; isSuccess: boolean } = useData(); // this uses a context provider to call data once and make it available to all children -- this may not be better than just using the query hooks, kind of does the same thing | |
| // get hydrophone feed list | |
| const feedsQueryResult = useFeedsQuery(); | |
| const feeds = feedsQueryResult.data?.feeds ?? []; | |
| const [filters, setFilters] = useState({ | |
| timeRange: threeDays, | |
| timeIncrement: 15, | |
| hydrophone: "All hydrophones", | |
| category: "All categories", | |
| }); | |
| // Update any function calls or references to use values directly from filters | |
| // rather than the individual state variables. | |
| } |
Summary by CodeRabbit
New Features
Chores / Refactor