feat(waveform): do waveform for audio#814
Merged
navin-moorthy merged 8 commits intodevfrom Mar 3, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a WaveSurfer-based AudioWaveformPlayer and Spotify embed; refactors MediaPlayer to separate audio/Spotify from video/YouTube; adds audio error tracking and Spotify URL parsing for the lightbox; moves styling into CSS tokens and removes TS style constants; updates icons, typings, deps, and small assets. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/React
participant AWP as AudioWaveformPlayer
participant WS as WaveSurfer
participant AE as HTMLAudioElement
participant MC as media-chrome
Client->>AWP: mount with `src`, `isActive`
AWP->>WS: lazy import & create instance
AWP->>AE: bind WaveSurfer to audio element & container
rect rgba(100, 200, 100, 0.5)
AE->>AWP: loadeddata/canplay -> set duration/ready
WS->>AWP: waveform rendered -> ready
AWP->>Client: expose ready state
end
rect rgba(100, 150, 200, 0.5)
MC->>AWP: play/pause/seek/volume actions
AWP->>AE: play()/pause()/currentTime/volume
AE->>WS: update waveform position
AWP->>MC: update UI (time, progress)
end
rect rgba(200, 150, 100, 0.5)
Client->>AWP: ArrowLeft/ArrowRight -> keyboard seek
Client->>AWP: unmount or src change
AWP->>WS: destroy & cleanup
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/MediaPlayer.tsx (1)
115-146:⚠️ Potential issue | 🔴 CriticalMemory leak: ref callback cleanup function is never invoked.
React ref callbacks don't return cleanup functions. The callback is invoked with the element on mount and with
nullon unmount—your returned cleanup function on lines 138-143 is silently discarded, so event listeners accumulate.🐛 Proposed fix using useEffect for event listeners
const ref = useCallback( (el: HTMLElement | null) => { mediaElRef.current = el; mediaRef(el as HTMLMediaElement | null); - - if (!el) { - return undefined; - } - - const handleError = () => onErrorRef.current?.(); - const handleLoaded = () => setLoading(false); - - // Check if media is already loaded (event fired before listener attached) - if ((el as HTMLMediaElement).readyState >= 2) { - setLoading(false); - } - - el.addEventListener("error", handleError); - // youtube-video-element may not fire `loadeddata`; listen for - // `loadedmetadata` as well so the spinner always clears. - el.addEventListener("loadedmetadata", handleLoaded); - el.addEventListener("loadeddata", handleLoaded); - - return () => { - el.removeEventListener("error", handleError); - el.removeEventListener("loadedmetadata", handleLoaded); - el.removeEventListener("loadeddata", handleLoaded); - mediaRef(null); - }; }, [mediaRef], ); + + useEffect(() => { + const el = mediaElRef.current; + if (!el) { + return; + } + + const handleError = () => onErrorRef.current?.(); + const handleLoaded = () => setLoading(false); + + // Check if media is already loaded (event fired before listener attached) + if ((el as HTMLMediaElement).readyState >= 2) { + setLoading(false); + } + + el.addEventListener("error", handleError); + el.addEventListener("loadedmetadata", handleLoaded); + el.addEventListener("loadeddata", handleLoaded); + + return () => { + el.removeEventListener("error", handleError); + el.removeEventListener("loadedmetadata", handleLoaded); + el.removeEventListener("loadeddata", handleLoaded); + }; + }, [mediaType, youTubeReady]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MediaPlayer.tsx` around lines 115 - 146, The ref callback (ref) is returning a cleanup function which React ignores, causing event listeners to accumulate; remove the returned cleanup from ref and instead add a useEffect that watches mediaElRef.current (and mediaRef if needed), attaches the same listeners (error -> onErrorRef.current, loadedmetadata/loadeddata -> setLoading(false)), performs the readyState >= 2 check there, and removes those listeners in the effect cleanup; keep calling mediaRef(el) in the ref callback but move handleError/handleLoaded listener registration and the removeEventListener logic into the new useEffect so listeners are always cleaned up when mediaElRef.current changes or on unmount.
🧹 Nitpick comments (4)
src/types/youtube-video-element.d.ts (1)
1-1: Consider usingimport typestatement instead of inline type modifiers.Both syntaxes achieve type-only imports, but
import typeis the more common form and aligns better with the codebase guideline.🔧 Suggested change
-import { type DetailedHTMLProps, type VideoHTMLAttributes } from "react"; +import type { DetailedHTMLProps, VideoHTMLAttributes } from "react";As per coding guidelines: "Always use
import typefor types".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/youtube-video-element.d.ts` at line 1, Replace the inline type modifiers in the import by using an `import type` declaration: change the existing import that brings in DetailedHTMLProps and VideoHTMLAttributes with inline `type` keywords to a single `import type` import for those symbols (DetailedHTMLProps, VideoHTMLAttributes) from "react" so the file's types are imported as type-only per the guideline.src/components/AudioWaveformPlayer.tsx (1)
25-25: Prefer path alias import here for consistency.Use the repo alias for component imports instead of relative paths.
Suggested fix
-import { MuteIcon, PlayPauseIcon, SettingsIcon } from "./media-player-icons"; +import { MuteIcon, PlayPauseIcon, SettingsIcon } from "@/components/media-player-icons";As per coding guidelines "Use path aliases (
@/components/,@/utils/) for imports instead of relative paths".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AudioWaveformPlayer.tsx` at line 25, The import in AudioWaveformPlayer.tsx currently uses a relative path for media-player-icons; change it to use the repo path alias instead (e.g., import { MuteIcon, PlayPauseIcon, SettingsIcon } from "@/components/media-player-icons") so the component import follows the project's path-alias convention; update the import statement in the AudioWaveformPlayer component accordingly.src/components/MediaPlayer.tsx (2)
251-257: Consider adding JSDoc for the exported function.As per coding guidelines, exported functions should include JSDoc comments documenting purpose and parameters.
📝 Suggested JSDoc
+/** + * Renders a media player based on the media type. + * Delegates to AudioWaveformPlayer for audio, SpotifyEmbed for Spotify, + * or VideoPlayerInner (with media-chrome) for video/YouTube content. + */ export function MediaPlayer({ isActive, mediaType, onError, src, title, }: MediaPlayerProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MediaPlayer.tsx` around lines 251 - 257, Add a JSDoc block above the exported MediaPlayer function that briefly describes its purpose (renders audio/video based on mediaType and active state), documents each parameter (isActive, mediaType, onError, src, title) including types/expected values and behavior, and notes the return type (JSX.Element). Reference the MediaPlayer function and MediaPlayerProps in the comment so consumers and IDEs get proper tooling/help.
31-40: Consider using path aliases consistently for all imports.Lines 31-39 use relative imports while line 40 uses
@/utils/. As per coding guidelines, prefer path aliases (@/components/) over relative paths for consistency.♻️ Suggested refactor
import { FullscreenIcon, MuteIcon, PipIcon, PlayPauseIcon, SettingsIcon, -} from "./media-player-icons"; -import { Spinner } from "./spinner"; -import { SpotifyEmbed } from "./SpotifyEmbed"; +} from "@/components/media-player-icons"; +import { Spinner } from "@/components/spinner"; +import { SpotifyEmbed } from "@/components/SpotifyEmbed"; import { cn } from "@/utils/tailwind-merge";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MediaPlayer.tsx` around lines 31 - 40, The imports at the top of MediaPlayer.tsx mix relative paths and a path alias; replace the relative imports for FullscreenIcon, MuteIcon, PipIcon, PlayPauseIcon, SettingsIcon, Spinner, and SpotifyEmbed with the corresponding alias form (e.g., import { FullscreenIcon } from "@/components/media-player-icons") so all imports use the "@/…" alias consistently and keep the existing import of cn from "@/utils/tailwind-merge"; update only the import statements, preserving the imported symbols and their usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/AudioWaveformPlayer.tsx`:
- Around line 229-235: The slider element in AudioWaveformPlayer is still
keyboard-focusable when not ready (isReady is false) because tabIndex is
hardcoded to 0; change the tabIndex behavior so it is focusable only when ready
(e.g., set tabIndex to 0 when isReady is true and -1 when false) and ensure any
keyboard handlers like handleWaveformKeyDown and role="slider" remain consistent
with that state to avoid hidden focus targets.
- Around line 215-217: The <track> element in AudioWaveformPlayer (audioRef,
src) is rendered without a valid captions source so it never loads; update
AudioWaveformPlayer to only render the <track> element when a captions source
prop is provided (e.g., captionsSrc or captionsUrl) or remove the placeholder
entirely, and apply the same conditional rendering change in MediaPlayer (the
<video> element around the existing track at the noted location) so captions are
only emitted when a real src is available.
In `@src/components/lightbox/LightboxRenderers.tsx`:
- Around line 121-125: The handleError callback incorrectly treats bookmark.id
=== 0 as falsy and skips calling onAudioError; update the condition in
handleError (the function using bookmark?.id and onAudioError) to perform a
strict numeric check such as verifying typeof bookmark?.id === "number" (or
bookmark.id !== undefined && typeof bookmark.id === "number") so id 0 is
accepted, then call onAudioError(bookmark.id) as before.
---
Outside diff comments:
In `@src/components/MediaPlayer.tsx`:
- Around line 115-146: The ref callback (ref) is returning a cleanup function
which React ignores, causing event listeners to accumulate; remove the returned
cleanup from ref and instead add a useEffect that watches mediaElRef.current
(and mediaRef if needed), attaches the same listeners (error ->
onErrorRef.current, loadedmetadata/loadeddata -> setLoading(false)), performs
the readyState >= 2 check there, and removes those listeners in the effect
cleanup; keep calling mediaRef(el) in the ref callback but move
handleError/handleLoaded listener registration and the removeEventListener logic
into the new useEffect so listeners are always cleaned up when
mediaElRef.current changes or on unmount.
---
Nitpick comments:
In `@src/components/AudioWaveformPlayer.tsx`:
- Line 25: The import in AudioWaveformPlayer.tsx currently uses a relative path
for media-player-icons; change it to use the repo path alias instead (e.g.,
import { MuteIcon, PlayPauseIcon, SettingsIcon } from
"@/components/media-player-icons") so the component import follows the project's
path-alias convention; update the import statement in the AudioWaveformPlayer
component accordingly.
In `@src/components/MediaPlayer.tsx`:
- Around line 251-257: Add a JSDoc block above the exported MediaPlayer function
that briefly describes its purpose (renders audio/video based on mediaType and
active state), documents each parameter (isActive, mediaType, onError, src,
title) including types/expected values and behavior, and notes the return type
(JSX.Element). Reference the MediaPlayer function and MediaPlayerProps in the
comment so consumers and IDEs get proper tooling/help.
- Around line 31-40: The imports at the top of MediaPlayer.tsx mix relative
paths and a path alias; replace the relative imports for FullscreenIcon,
MuteIcon, PipIcon, PlayPauseIcon, SettingsIcon, Spinner, and SpotifyEmbed with
the corresponding alias form (e.g., import { FullscreenIcon } from
"@/components/media-player-icons") so all imports use the "@/…" alias
consistently and keep the existing import of cn from "@/utils/tailwind-merge";
update only the import statements, preserving the imported symbols and their
usage.
In `@src/types/youtube-video-element.d.ts`:
- Line 1: Replace the inline type modifiers in the import by using an `import
type` declaration: change the existing import that brings in DetailedHTMLProps
and VideoHTMLAttributes with inline `type` keywords to a single `import type`
import for those symbols (DetailedHTMLProps, VideoHTMLAttributes) from "react"
so the file's types are imported as type-only per the guideline.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/audio-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
.cspell/project-words.txtpackage.jsonsrc/components/AudioWaveformPlayer.tsxsrc/components/MediaPlayer.tsxsrc/components/SpotifyEmbed.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsxsrc/components/lightbox/LightboxUtils.tsxsrc/components/media-player-icons.tsxsrc/components/media-player-theme.csssrc/components/media-player-theme.tssrc/icons/actionIcons/audioIcon.tsxsrc/types/youtube-video-element.d.tssrc/utils/constants.ts
💤 Files with no reviewable changes (3)
- src/icons/actionIcons/audioIcon.tsx
- .cspell/project-words.txt
- src/components/media-player-theme.ts
Comment on lines
+215
to
+217
| <audio ref={audioRef} slot="media" src={src}> | ||
| <track default kind="captions" label="No captions" srcLang="en" /> | ||
| </audio> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for track elements missing src attribute across TSX files
rg -nP '<track\b(?![^>]*\bsrc=)' --type tsxRepository: timelessco/recollect
Length of output: 91
🏁 Script executed:
# Examine the AudioWaveformPlayer component
cat -n src/components/AudioWaveformPlayer.tsx | head -230 | tail -50Repository: timelessco/recollect
Length of output: 2136
🏁 Script executed:
# Check the full component structure to understand props
cat -n src/components/AudioWaveformPlayer.tsx | head -50Repository: timelessco/recollect
Length of output: 1598
🏁 Script executed:
# Search for track elements with src attribute in JS/TS files
rg '<track[^>]*src=' --type-add 'ts:*.{ts,tsx,js,jsx}' -t tsRepository: timelessco/recollect
Length of output: 46
🏁 Script executed:
# Find usages of AudioWaveformPlayer component
rg 'AudioWaveformPlayer' --type-add 'ts:*.{ts,tsx,js,jsx}' -t ts -B 2 -A 5Repository: timelessco/recollect
Length of output: 2911
🏁 Script executed:
# Check for other audio/video elements with track tags
rg '<(audio|video)[^>]*>.*<track' --type-add 'ts:*.{ts,tsx,js,jsx}' -t ts -A 2Repository: timelessco/recollect
Length of output: 46
🏁 Script executed:
# Search for all track elements in the codebase
rg '<track' --type-add 'ts:*.{ts,tsx,js,jsx}' -t tsRepository: timelessco/recollect
Length of output: 276
🏁 Script executed:
# Check the MediaPlayer component where AudioWaveformPlayer is used
cat -n src/components/MediaPlayer.tsx | grep -A 10 'AudioWaveformPlayer'Repository: timelessco/recollect
Length of output: 764
🏁 Script executed:
# Look for caption or subtitle data patterns
rg 'caption|subtitle' --type-add 'ts:*.{ts,tsx,js,jsx}' -t ts -iRepository: timelessco/recollect
Length of output: 8133
🏁 Script executed:
# Check MediaPlayer component's track element
cat -n src/components/MediaPlayer.tsx | grep -B 5 -A 2 'track default'Repository: timelessco/recollect
Length of output: 333
🏁 Script executed:
# Verify the full context of how both components receive their props
head -60 src/components/MediaPlayer.tsx | tail -20Repository: timelessco/recollect
Length of output: 487
🏁 Script executed:
# Check if there's any caption/subtitle data available in the bookmark/media data structures
rg 'audio_caption|video_caption|subtitle' --type-add 'ts:*.{ts,tsx,js,jsx}' -t ts -iRepository: timelessco/recollect
Length of output: 407
🏁 Script executed:
# Verify the complete context around line 215-217 in AudioWaveformPlayer
sed -n '210,225p' src/components/AudioWaveformPlayer.tsxRepository: timelessco/recollect
Length of output: 970
Caption track is non-functional without a src URL.
The <track> element won't load captions as written. This same issue also exists in src/components/MediaPlayer.tsx (line 168) with the <video> element. Consider either removing these non-functional placeholders or making the caption source optional and conditional on availability.
Suggested fix
export interface AudioWaveformPlayerProps {
+ captionSrc?: string;
isActive?: boolean;
onError?: () => void;
src: string;
title?: string;
}
@@
<audio ref={audioRef} slot="media" src={src}>
- <track default kind="captions" label="No captions" srcLang="en" />
+ {props.captionSrc ? (
+ <track
+ default
+ kind="captions"
+ label="English captions"
+ src={props.captionSrc}
+ srcLang="en"
+ />
+ ) : null}
</audio>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/AudioWaveformPlayer.tsx` around lines 215 - 217, The <track>
element in AudioWaveformPlayer (audioRef, src) is rendered without a valid
captions source so it never loads; update AudioWaveformPlayer to only render the
<track> element when a captions source prop is provided (e.g., captionsSrc or
captionsUrl) or remove the placeholder entirely, and apply the same conditional
rendering change in MediaPlayer (the <video> element around the existing track
at the noted location) so captions are only emitted when a real src is
available.
Contributor
Author
There was a problem hiding this comment.
Media elements such as and
Contributor
There was a problem hiding this comment.
@rogerantony-dev, thank you for the clarification! You're absolutely right—the <track> element is required to satisfy the jsx-a11y/media-has-caption ESLint rule, which enforces accessibility best practices by ensuring media elements have caption tracks.
This aligns with the accessibility guidelines in this repository. While the track doesn't have an actual caption file source yet, having the placeholder element satisfies the linting requirement and ensures the component structure is ready if caption sources become available in the future.
The current implementation is appropriate for meeting accessibility tooling requirements. 👍
🧠 Learnings used
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include caption tracks for audio and video elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure label elements have text content and are associated with an input
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure anchors have content that's accessible to screen readers
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always give all elements requiring alt text meaningful information for screen readers
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure ARIA properties are valid for the element's supported roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always use valid, non-abstract ARIA roles for elements with ARIA roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include a `title` attribute for iframe elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Never assign non-interactive ARIA roles to interactive HTML elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always give heading elements content that's accessible to screen readers (not hidden with `aria-hidden`)
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Never assign interactive ARIA roles to non-interactive HTML elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always use semantic elements instead of role attributes in JSX
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include a `title` element for SVG elements
Learnt from: navin-moorthy
Repo: timelessco/recollect PR: 694
File: src/components/lightbox/AddToCollectionDropdown.tsx:163-167
Timestamp: 2025-12-16T11:52:03.069Z
Learning: When reviewing code for react-aria-components usage, check for nightly build API changes in Select: replace selectedKeys/onSelectionChange with value/onChange. For single-select, value should be a scalar representing the item ID; for multi-select, value should be an array of item IDs and onChange should receive the updated array. Update any references in src/components/...tsx files accordingly (e.g., AddToCollectionDropdown.tsx) to use value and onChange with the appropriate types, and remove any remaining selectedKeys or onSelectionChange usage.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 732
File: src/components/toggleDarkMode.tsx:5-5
Timestamp: 2026-01-08T07:01:43.857Z
Learning: Within the repository where the settings modal is built using Ariakit Dialog (src/components/modal.tsx), prefer using Ariakit components (e.g., AriaDropdown) inside the modal to maintain accessibility, consistent styling, and reduce custom wiring. This guidance applies to TSX files under src/components that render modals or settings dialogs.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 722
File: src/pageComponents/settings/import.tsx:114-121
Timestamp: 2026-01-22T06:37:54.068Z
Learning: When reviewing React components that use React Query mutations (e.g., useImportBookmarksMutation), errors from the API or network are handled inside the mutation hook. Do not wrap the mutation call in a broad component-level try/catch for API errors; instead, handle only local, pre-mutation errors (such as CSV parsing or client-side validation) at the call site. Ensure the UI responds to mutation state (isLoading, isError, error) and presents user-friendly messages for mutation failures provided by the hook.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 747
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:40-41
Timestamp: 2026-02-04T05:04:02.699Z
Learning: Tailwind CSS v4 arbitrary spacing values are supported. In this repository (Tailwind v4.1.18), numeric spacing classes like -ml-0.25 or mt-21 are valid and map to N * 0.25rem (i.e., -ml-0.25 => -0.0625rem, mt-21 => 5.25rem). When reviewing TSX files, accept or apply these numeric spacing utilities as needed, ensuring the chosen values align with the design scale and responsive behavior.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 806
File: src/components/ui/recollect/tooltip.tsx:1-1
Timestamp: 2026-02-25T08:14:22.185Z
Learning: In the timelessco/recollect repo, enforce kebab-case for component file names across all .ts and .tsx files. For example, tooltip.tsx and scroll-area.tsx should be used; avoid camelCase or PascalCase such as Tooltip.tsx or ScrollArea.tsx. This improves consistency and import readability. Apply to all files matching the pattern **/*.{ts,tsx}.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 810
File: src/pageComponents/dashboard/share/share-public-switch.tsx:39-58
Timestamp: 2026-02-26T07:28:41.161Z
Learning: In the timelessco/recollect repo, when a Base UI Switch is rendered inside a menu/dropdown context (e.g., AriaDropdown), the onCheckedChange handler may not fire due to the menu's click handler closing the dropdown. Actionable workarounds: (1) use onClick instead of onCheckedChange, (2) set closeOnClick={false} to prevent the menu from closing on click, (3) call event.stopPropagation() in the handler to prevent the menu's click handler from triggering closure. This guideline applies to all .tsx files containing Switch components rendered inside menu/dropdown contexts across the codebase.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 812
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:26-27
Timestamp: 2026-02-27T09:46:21.070Z
Learning: When reviewing focused refactoring PRs (such as UI library migrations or component replacements), do not flag pre-existing code quality issues (such as missing error handling, performance concerns, or architectural problems) as blocking concerns if they fall outside the stated scope of the refactor. Keep reviews focused on the refactoring objectives. Suggestions for improvements to pre-existing code should be marked as non-blocking, optional, or deferred to separate follow-up issues. This maintains PR focus and prevents scope creep while still documenting areas for future improvement.
Contributor
Author
There was a problem hiding this comment.
Already resolved — the <track> is required by the jsx-a11y/media-has-caption ESLint rule. No real captions source exists yet, so the placeholder element is the correct approach.
Contributor
Author
There was a problem hiding this comment.
Already resolved — the <track> is required by the jsx-a11y/media-has-caption ESLint rule. No real captions source exists yet, so the placeholder element is the correct approach.
Contributor
There was a problem hiding this comment.
@rogerantony-dev, understood! Thank you for the clarification. The placeholder <track> element is indeed the right approach to satisfy the ESLint rule while the component structure remains ready for future caption sources. 👍
🧠 Learnings used
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include caption tracks for audio and video elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure anchors have content that's accessible to screen readers
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always use valid, non-abstract ARIA roles for elements with ARIA roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure label elements have text content and are associated with an input
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always give all elements requiring alt text meaningful information for screen readers
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure ARIA properties are valid for the element's supported roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always give heading elements content that's accessible to screen readers (not hidden with `aria-hidden`)
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include all required ARIA attributes for elements with ARIA roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Never assign interactive ARIA roles to non-interactive HTML elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Never assign non-interactive ARIA roles to interactive HTML elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include a `title` attribute for iframe elements
Learnt from: navin-moorthy
Repo: timelessco/recollect PR: 694
File: src/components/lightbox/AddToCollectionDropdown.tsx:163-167
Timestamp: 2025-12-16T11:52:03.069Z
Learning: When reviewing code for react-aria-components usage, check for nightly build API changes in Select: replace selectedKeys/onSelectionChange with value/onChange. For single-select, value should be a scalar representing the item ID; for multi-select, value should be an array of item IDs and onChange should receive the updated array. Update any references in src/components/...tsx files accordingly (e.g., AddToCollectionDropdown.tsx) to use value and onChange with the appropriate types, and remove any remaining selectedKeys or onSelectionChange usage.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 732
File: src/components/toggleDarkMode.tsx:5-5
Timestamp: 2026-01-08T07:01:43.857Z
Learning: Within the repository where the settings modal is built using Ariakit Dialog (src/components/modal.tsx), prefer using Ariakit components (e.g., AriaDropdown) inside the modal to maintain accessibility, consistent styling, and reduce custom wiring. This guidance applies to TSX files under src/components that render modals or settings dialogs.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 722
File: src/pageComponents/settings/import.tsx:114-121
Timestamp: 2026-01-22T06:37:54.068Z
Learning: When reviewing React components that use React Query mutations (e.g., useImportBookmarksMutation), errors from the API or network are handled inside the mutation hook. Do not wrap the mutation call in a broad component-level try/catch for API errors; instead, handle only local, pre-mutation errors (such as CSV parsing or client-side validation) at the call site. Ensure the UI responds to mutation state (isLoading, isError, error) and presents user-friendly messages for mutation failures provided by the hook.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 747
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:40-41
Timestamp: 2026-02-04T05:04:02.699Z
Learning: Tailwind CSS v4 arbitrary spacing values are supported. In this repository (Tailwind v4.1.18), numeric spacing classes like -ml-0.25 or mt-21 are valid and map to N * 0.25rem (i.e., -ml-0.25 => -0.0625rem, mt-21 => 5.25rem). When reviewing TSX files, accept or apply these numeric spacing utilities as needed, ensuring the chosen values align with the design scale and responsive behavior.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 806
File: src/components/ui/recollect/tooltip.tsx:1-1
Timestamp: 2026-02-25T08:14:22.185Z
Learning: In the timelessco/recollect repo, enforce kebab-case for component file names across all .ts and .tsx files. For example, tooltip.tsx and scroll-area.tsx should be used; avoid camelCase or PascalCase such as Tooltip.tsx or ScrollArea.tsx. This improves consistency and import readability. Apply to all files matching the pattern **/*.{ts,tsx}.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 810
File: src/pageComponents/dashboard/share/share-public-switch.tsx:39-58
Timestamp: 2026-02-26T07:28:41.161Z
Learning: In the timelessco/recollect repo, when a Base UI Switch is rendered inside a menu/dropdown context (e.g., AriaDropdown), the onCheckedChange handler may not fire due to the menu's click handler closing the dropdown. Actionable workarounds: (1) use onClick instead of onCheckedChange, (2) set closeOnClick={false} to prevent the menu from closing on click, (3) call event.stopPropagation() in the handler to prevent the menu's click handler from triggering closure. This guideline applies to all .tsx files containing Switch components rendered inside menu/dropdown contexts across the codebase.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 812
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:26-27
Timestamp: 2026-02-27T09:46:21.070Z
Learning: When reviewing focused refactoring PRs (such as UI library migrations or component replacements), do not flag pre-existing code quality issues (such as missing error handling, performance concerns, or architectural problems) as blocking concerns if they fall outside the stated scope of the refactor. Keep reviews focused on the refactoring objectives. Suggestions for improvements to pre-existing code should be marked as non-blocking, optional, or deferred to separate follow-up issues. This maintains PR focus and prevents scope creep while still documenting areas for future improvement.
Contributor
There was a problem hiding this comment.
@rogerantony-dev, understood! Thank you for the confirmation. You're absolutely right—the ESLint rule is satisfied when a <track> tag is present, even without a src attribute. The placeholder track element correctly fulfills the jsx-a11y/media-has-caption requirement while the component awaits actual caption sources in the future.
I appreciate the clarification on the intentional design here. 👍
🧠 Learnings used
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include caption tracks for audio and video elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure anchors have content that's accessible to screen readers
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always use valid, non-abstract ARIA roles for elements with ARIA roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure label elements have text content and are associated with an input
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always give all elements requiring alt text meaningful information for screen readers
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always ensure ARIA properties are valid for the element's supported roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always give heading elements content that's accessible to screen readers (not hidden with `aria-hidden`)
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include all required ARIA attributes for elements with ARIA roles
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Never assign interactive ARIA roles to non-interactive HTML elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Never assign non-interactive ARIA roles to interactive HTML elements
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include a `title` attribute for iframe elements
Learnt from: navin-moorthy
Repo: timelessco/recollect PR: 694
File: src/components/lightbox/AddToCollectionDropdown.tsx:163-167
Timestamp: 2025-12-16T11:52:03.069Z
Learning: When reviewing code for react-aria-components usage, check for nightly build API changes in Select: replace selectedKeys/onSelectionChange with value/onChange. For single-select, value should be a scalar representing the item ID; for multi-select, value should be an array of item IDs and onChange should receive the updated array. Update any references in src/components/...tsx files accordingly (e.g., AddToCollectionDropdown.tsx) to use value and onChange with the appropriate types, and remove any remaining selectedKeys or onSelectionChange usage.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 732
File: src/components/toggleDarkMode.tsx:5-5
Timestamp: 2026-01-08T07:01:43.857Z
Learning: Within the repository where the settings modal is built using Ariakit Dialog (src/components/modal.tsx), prefer using Ariakit components (e.g., AriaDropdown) inside the modal to maintain accessibility, consistent styling, and reduce custom wiring. This guidance applies to TSX files under src/components that render modals or settings dialogs.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 722
File: src/pageComponents/settings/import.tsx:114-121
Timestamp: 2026-01-22T06:37:54.068Z
Learning: When reviewing React components that use React Query mutations (e.g., useImportBookmarksMutation), errors from the API or network are handled inside the mutation hook. Do not wrap the mutation call in a broad component-level try/catch for API errors; instead, handle only local, pre-mutation errors (such as CSV parsing or client-side validation) at the call site. Ensure the UI responds to mutation state (isLoading, isError, error) and presents user-friendly messages for mutation failures provided by the hook.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 747
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:40-41
Timestamp: 2026-02-04T05:04:02.699Z
Learning: Tailwind CSS v4 arbitrary spacing values are supported. In this repository (Tailwind v4.1.18), numeric spacing classes like -ml-0.25 or mt-21 are valid and map to N * 0.25rem (i.e., -ml-0.25 => -0.0625rem, mt-21 => 5.25rem). When reviewing TSX files, accept or apply these numeric spacing utilities as needed, ensuring the chosen values align with the design scale and responsive behavior.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 806
File: src/components/ui/recollect/tooltip.tsx:1-1
Timestamp: 2026-02-25T08:14:22.185Z
Learning: In the timelessco/recollect repo, enforce kebab-case for component file names across all .ts and .tsx files. For example, tooltip.tsx and scroll-area.tsx should be used; avoid camelCase or PascalCase such as Tooltip.tsx or ScrollArea.tsx. This improves consistency and import readability. Apply to all files matching the pattern **/*.{ts,tsx}.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 810
File: src/pageComponents/dashboard/share/share-public-switch.tsx:39-58
Timestamp: 2026-02-26T07:28:41.161Z
Learning: In the timelessco/recollect repo, when a Base UI Switch is rendered inside a menu/dropdown context (e.g., AriaDropdown), the onCheckedChange handler may not fire due to the menu's click handler closing the dropdown. Actionable workarounds: (1) use onClick instead of onCheckedChange, (2) set closeOnClick={false} to prevent the menu from closing on click, (3) call event.stopPropagation() in the handler to prevent the menu's click handler from triggering closure. This guideline applies to all .tsx files containing Switch components rendered inside menu/dropdown contexts across the codebase.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 812
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:26-27
Timestamp: 2026-02-27T09:46:21.070Z
Learning: When reviewing focused refactoring PRs (such as UI library migrations or component replacements), do not flag pre-existing code quality issues (such as missing error handling, performance concerns, or architectural problems) as blocking concerns if they fall outside the stated scope of the refactor. Keep reviews focused on the refactoring objectives. Suggestions for improvements to pre-existing code should be marked as non-blocking, optional, or deferred to separate follow-up issues. This maintains PR focus and prevents scope creep while still documenting areas for future improvement.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/AudioWaveformPlayer.tsx (1)
229-235:⚠️ Potential issue | 🟡 MinorStill needs fix: Invisible slider is keyboard-focusable.
The previous review correctly identified that when
!isReady, the slider is visually hidden but still focusable withtabIndex={0}. This creates a confusing keyboard navigation experience.🛠️ Apply the previously suggested fix
<div ref={containerRef} aria-label="Audio progress" aria-valuemax={duration} aria-valuemin={0} aria-valuenow={currentTime} className={cn( "h-12 flex-1 cursor-pointer touch-none rounded-lg outline-none focus-visible:ring-2 focus-visible:ring-blue-500", !isReady && "invisible absolute", )} onKeyDown={handleWaveformKeyDown} role="slider" - tabIndex={0} + tabIndex={isReady ? 0 : -1} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AudioWaveformPlayer.tsx` around lines 229 - 235, The slider element in AudioWaveformPlayer is visually hidden when !isReady but remains keyboard-focusable; update the element that currently uses tabIndex={0}, role="slider", and onKeyDown={handleWaveformKeyDown} so that when !isReady it sets tabIndex={-1} and aria-hidden="true" (or removes the interactive semantics) to prevent keyboard focus, otherwise keep tabIndex={0} and role="slider"; adjust the conditional logic around isReady to apply these attributes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/AudioWaveformPlayer.tsx`:
- Around line 229-235: The slider element in AudioWaveformPlayer is visually
hidden when !isReady but remains keyboard-focusable; update the element that
currently uses tabIndex={0}, role="slider", and
onKeyDown={handleWaveformKeyDown} so that when !isReady it sets tabIndex={-1}
and aria-hidden="true" (or removes the interactive semantics) to prevent
keyboard focus, otherwise keep tabIndex={0} and role="slider"; adjust the
conditional logic around isReady to apply these attributes accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/AudioWaveformPlayer.tsx (1)
229-235:⚠️ Potential issue | 🟡 MinorMake the waveform slider unfocusable until it is ready.
When
!isReady, the element is visually hidden but Line 234 still keeps it in tab order (tabIndex={0}), creating a hidden focus target.💡 Proposed fix
- tabIndex={0} + tabIndex={isReady ? 0 : -1}As per coding guidelines: "Ensure all interactive elements are keyboard accessible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AudioWaveformPlayer.tsx` around lines 229 - 235, The waveform slider in AudioWaveformPlayer is visually hidden when !isReady but remains focusable because tabIndex is hardcoded to 0; update the element that renders the slider (the element using onKeyDown={handleWaveformKeyDown}, role="slider") to set tabIndex conditionally so it is unfocusable when not ready (e.g., tabIndex={isReady ? 0 : -1}), and you may also mirror readiness with an appropriate aria-disabled or aria-hidden attribute to keep accessibility state consistent.
🧹 Nitpick comments (2)
src/components/MediaPlayer.tsx (1)
39-39: Use alias import forSpotifyEmbed.Please replace the new relative import with the project alias convention.
💡 Proposed fix
-import { SpotifyEmbed } from "./SpotifyEmbed"; +import { SpotifyEmbed } from "@/components/SpotifyEmbed";As per coding guidelines: "Use path aliases (
@/components/,@/utils/) for imports instead of relative paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MediaPlayer.tsx` at line 39, The import in MediaPlayer.tsx uses a relative path for SpotifyEmbed; change it to the project alias form (e.g., import { SpotifyEmbed } from "@/components/SpotifyEmbed";) so the file imports follow the path-alias convention; update the import statement referencing SpotifyEmbed in the MediaPlayer component to use the "@/components/..." alias.src/components/AudioWaveformPlayer.tsx (1)
25-25: Use the project alias for this component import.Please switch
./media-player-iconsto the@/components/...alias to keep import style consistent.💡 Proposed fix
-import { MuteIcon, PlayPauseIcon, SettingsIcon } from "./media-player-icons"; +import { MuteIcon, PlayPauseIcon, SettingsIcon } from "@/components/media-player-icons";As per coding guidelines: "Use path aliases (
@/components/,@/utils/) for imports instead of relative paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AudioWaveformPlayer.tsx` at line 25, The import in AudioWaveformPlayer.tsx currently pulls MuteIcon, PlayPauseIcon, and SettingsIcon from a relative path ("./media-player-icons"); update that import to use the project path alias (e.g., "@/components/media-player-icons") so the file imports MuteIcon, PlayPauseIcon, and SettingsIcon via the alias and matches the project's coding guidelines for using "@/components/" instead of relative paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/AudioWaveformPlayer.tsx`:
- Around line 229-235: The waveform slider in AudioWaveformPlayer is visually
hidden when !isReady but remains focusable because tabIndex is hardcoded to 0;
update the element that renders the slider (the element using
onKeyDown={handleWaveformKeyDown}, role="slider") to set tabIndex conditionally
so it is unfocusable when not ready (e.g., tabIndex={isReady ? 0 : -1}), and you
may also mirror readiness with an appropriate aria-disabled or aria-hidden
attribute to keep accessibility state consistent.
---
Nitpick comments:
In `@src/components/AudioWaveformPlayer.tsx`:
- Line 25: The import in AudioWaveformPlayer.tsx currently pulls MuteIcon,
PlayPauseIcon, and SettingsIcon from a relative path ("./media-player-icons");
update that import to use the project path alias (e.g.,
"@/components/media-player-icons") so the file imports MuteIcon, PlayPauseIcon,
and SettingsIcon via the alias and matches the project's coding guidelines for
using "@/components/" instead of relative paths.
In `@src/components/MediaPlayer.tsx`:
- Line 39: The import in MediaPlayer.tsx uses a relative path for SpotifyEmbed;
change it to the project alias form (e.g., import { SpotifyEmbed } from
"@/components/SpotifyEmbed";) so the file imports follow the path-alias
convention; update the import statement referencing SpotifyEmbed in the
MediaPlayer component to use the "@/components/..." alias.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/AudioWaveformPlayer.tsxsrc/components/MediaPlayer.tsxsrc/components/media-player-icons.tsxsrc/components/media-player-theme.csssrc/styles/globals.css
✅ Files skipped from review due to trivial changes (1)
- src/styles/globals.css
Contributor
Author
Re: Outside-diff comment —
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/MediaPlayer.tsx`:
- Around line 123-149: The MediaPlayer component’s loading state isn’t reset
when src changes because the component instance is reused; update the parent
slide components (e.g., VideoSlide and AudioSlide where
VideoPlayerInner/MediaPlayer are rendered) to force a remount by adding a key
prop based on the unique media identifier (for example key={bookmark?.id}) to
the MediaPlayer/VideoPlayerInner element so the component unmounts and
re-initializes its loading state when the src/bookmark changes; update all media
slide components that render MediaPlayer to follow this pattern.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/AudioWaveformPlayer.tsxsrc/components/MediaPlayer.tsxsrc/components/lightbox/LightboxRenderers.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/AudioWaveformPlayer.tsx
- src/components/lightbox/LightboxRenderers.tsx
Comment on lines
+123
to
+149
| useEffect(() => { | ||
| const el = mediaElRef.current; | ||
| if (!el) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const handleError = () => onErrorRef.current?.(); | ||
| const handleLoaded = () => setLoading(false); | ||
|
|
||
| // Check if media is already loaded (event fired before listener attached) | ||
| if ((el as HTMLMediaElement).readyState >= 2) { | ||
| setLoading(false); | ||
| } | ||
|
|
||
| el.addEventListener("error", handleError); | ||
| // youtube-video-element may not fire `loadeddata`; listen for | ||
| // `loadedmetadata` as well so the spinner always clears. | ||
| el.addEventListener("loadedmetadata", handleLoaded); | ||
| el.addEventListener("loadeddata", handleLoaded); | ||
|
|
||
| return () => { | ||
| el.removeEventListener("error", handleError); | ||
| el.removeEventListener("loadedmetadata", handleLoaded); | ||
| el.removeEventListener("loadeddata", handleLoaded); | ||
| }; | ||
| }, [mediaType, youTubeReady]); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the MediaPlayer component
fd -t f 'MediaPlayer.tsx' -x wc -l {}Repository: timelessco/recollect
Length of output: 100
🏁 Script executed:
# Find the MediaPlayer component
fd -t f 'MediaPlayer.tsx'Repository: timelessco/recollect
Length of output: 94
🏁 Script executed:
# Search for MediaPlayer component usages in the codebase
rg -nP --type=tsx '<MediaPlayer\b' -C 2Repository: timelessco/recollect
Length of output: 91
🏁 Script executed:
# Read the MediaPlayer component
cat -n ./src/components/MediaPlayer.tsxRepository: timelessco/recollect
Length of output: 8837
🏁 Script executed:
# Search for MediaPlayer usages (without incorrect file type flag)
rg '<MediaPlayer' --type-list | grep -i tsxRepository: timelessco/recollect
Length of output: 453
🏁 Script executed:
# Try simpler search for MediaPlayer usages
rg '<MediaPlayer' -C 2Repository: timelessco/recollect
Length of output: 1757
🏁 Script executed:
# Check how MediaPlayer is used in LightboxRenderers.tsx
cat -n ./src/components/lightbox/LightboxRenderers.tsx | head -100Repository: timelessco/recollect
Length of output: 3584
🏁 Script executed:
# Search for the lightbox component to understand if MediaPlayer is reused with changing src
rg -A 10 -B 5 'class.*Lightbox|function.*Lightbox' ./src/components/lightbox/Repository: timelessco/recollect
Length of output: 1375
🏁 Script executed:
# Read more of LightboxRenderers.tsx to see all slide components
cat -n ./src/components/lightbox/LightboxRenderers.tsx | tail -80Repository: timelessco/recollect
Length of output: 2328
🏁 Script executed:
# Find the Lightbox component that uses these slides
rg -n 'slides.*=|render.*Slide' ./src/components/lightbox/ -A 3 -B 3Repository: timelessco/recollect
Length of output: 4362
🏁 Script executed:
# Check if there's a key prop being used on MediaPlayer or slide components
rg -n 'key=' ./src/components/lightbox/LightboxRenderers.tsxRepository: timelessco/recollect
Length of output: 46
🏁 Script executed:
# Find useLightboxSlides hook to understand slide structure
rg -n 'useLightboxSlides' ./src/components/lightbox/ -A 20Repository: timelessco/recollect
Length of output: 4756
🏁 Script executed:
# Look for the actual render implementation and how MediaPlayer/VideoSlide gets recreated
cat -n ./src/components/lightbox/LightBox.tsx | sed -n '131,180p'Repository: timelessco/recollect
Length of output: 1925
🏁 Script executed:
# Check if slides are being keyed by any identifier
rg -n 'index|key|bookmark\.id' ./src/components/lightbox/LightBoxPlugin.tsx -B 2 -A 2Repository: timelessco/recollect
Length of output: 473
🏁 Script executed:
# Check how VideoSlide and other media slides are rendered - look for key prop
cat -n ./src/components/lightbox/LightBox.tsx | sed -n '145,210p'Repository: timelessco/recollect
Length of output: 2365
🏁 Script executed:
# Look for CustomSlide type definition to understand slide structure
rg -n 'interface CustomSlide|type CustomSlide' ./src/components/lightbox/ -A 10Repository: timelessco/recollect
Length of output: 3732
🏁 Script executed:
# Check if there are other patterns of using key prop for state reset in the codebase
rg 'key=.*bookmark\.id|key={.*\.id}' ./src/components/ -B 2 -A 2Repository: timelessco/recollect
Length of output: 201
🏁 Script executed:
# Look for similar patterns of resetting state on prop changes elsewhere
rg 'useEffect.*\[.*,.*\]' ./src/components/lightbox/ | head -20Repository: timelessco/recollect
Length of output: 46
🏁 Script executed:
# Simpler search for key prop usage on components
rg 'key=' ./src/components/ | grep -E 'key=\{.*id|key=".*"' | head -10Repository: timelessco/recollect
Length of output: 873
🏁 Script executed:
# Check if there are examples of using key prop to force remount in the codebase
rg -n 'key=\{' ./src/components/lightbox/ -B 1 -A 1Repository: timelessco/recollect
Length of output: 1386
🏁 Script executed:
# Look at the actual lightbox library usage to understand if components are remounted or just updated
cat -n ./src/components/lightbox/LightBox.tsx | sed -n '420,450p'Repository: timelessco/recollect
Length of output: 602
🏁 Script executed:
# Verify how VideoSlide, AudioSlide etc render MediaPlayer - check full snippet
cat -n ./src/components/lightbox/LightboxRenderers.tsx | sed -n '77,110p'Repository: timelessco/recollect
Length of output: 1179
🏁 Script executed:
# Check other media slides for key prop pattern
cat -n ./src/components/lightbox/LightboxRenderers.tsx | sed -n '110,180p'Repository: timelessco/recollect
Length of output: 2378
🏁 Script executed:
# Final verification: Check if there are any comments or patterns about resetting component state in media players
rg -n 'loading.*reset|reset.*loading|key=.*src|remount' ./src/components/ | head -10Repository: timelessco/recollect
Length of output: 46
🏁 Script executed:
# Verify the actual behavior - when does VideoPlayerInner get a new src?
cat -n ./src/components/lightbox/LightBox.tsx | sed -n '195,205p'Repository: timelessco/recollect
Length of output: 332
Add key prop to MediaPlayer to reset loading state when src changes.
The loading state is not reset when src changes on the same mounted component instance. When navigating slides in the lightbox, VideoPlayerInner receives different src values but the loading spinner never appears again because the state was set to false from the previous load.
Instead of adding src to the useEffect dependency array, follow the codebase pattern of using the key prop to force component remount and naturally reset all state. Add key={bookmark?.id} to the MediaPlayer component in VideoSlide (line 98), AudioSlide (line 129), and other media slide components.
Per coding guidelines: "Reset state when props change using the key prop or calculate during render instead of useEffect".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/MediaPlayer.tsx` around lines 123 - 149, The MediaPlayer
component’s loading state isn’t reset when src changes because the component
instance is reused; update the parent slide components (e.g., VideoSlide and
AudioSlide where VideoPlayerInner/MediaPlayer are rendered) to force a remount
by adding a key prop based on the unique media identifier (for example
key={bookmark?.id}) to the MediaPlayer/VideoPlayerInner element so the component
unmounts and re-initializes its loading state when the src/bookmark changes;
update all media slide components that render MediaPlayer to follow this
pattern.
Contributor
Author
There was a problem hiding this comment.
The scenario it describes — same component instance receiving a different src because the bookmark changed — cannot happen here. VideoSlide/AudioSlide will always receive the same bookmark for the lifetime of that slide object. The loading state resetting to true between navigations is also not needed: when YARL pre-renders the "next" slide (with preload: 1), the video quietly loads in the background, and loading correctly transitions to false before the user even swipes to it — that's the intended behavior.
No changes needed. Adding key={bookmark?.id} to inside the slide components would just force unnecessary unmount/remount cycles on navigation, discarding pre-loaded state without fixing an actual problem.
Contributor
There was a problem hiding this comment.
@rogerantony-dev, thank you for the clarification! You're absolutely right—YARL preloads (2 * preload + 1) slides by default, so each VideoSlide/AudioSlide component maintains its own bookmark throughout its lifetime and the MediaPlayer never receives a different src prop for the same component instance. The loading state correctly transitions to false during background preload, which is exactly the intended UX. Adding a key prop would indeed force unnecessary remounts and discard the pre-loaded state. My original suggestion was based on a misunderstanding of YARL's slide management architecture.
✏️ Learnings added
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 814
File: src/components/MediaPlayer.tsx:123-149
Timestamp: 2026-03-02T11:50:49.320Z
Learning: In the timelessco/recollect repo using Yet Another React Lightbox (YARL), each slide component (VideoSlide, AudioSlide, etc.) maintains its own bookmark instance throughout its lifetime. YARL preloads adjacent slides in the background, so MediaPlayer components never receive different src values during their mounted lifetime. The loading state transitioning to false during background preload is the intended behavior. Do not suggest adding key props to MediaPlayer/slide components to "reset" state on navigation, as this would force unnecessary unmount/remount cycles and discard pre-loaded media state.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/code-style.mdc:0-0
Timestamp: 2026-02-26T09:24:42.970Z
Learning: Applies to **/*.{ts,tsx} : Use event handlers for user events instead of useEffect
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/code-style.mdc:0-0
Timestamp: 2026-02-26T09:24:42.970Z
Learning: Applies to **/*.{ts,tsx} : Reset state when props change using the key prop or calculate during render instead of useEffect
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 755
File: src/async/mutationHooks/bookmarks/use-move-bookmark-to-trash-optimistic-mutation.ts:115-131
Timestamp: 2026-02-05T10:17:04.808Z
Learning: In src/async/mutationHooks/bookmarks/use-move-bookmark-to-trash-optimistic-mutation.ts, when restoring bookmarks from trash, missing destination cache invalidation in onSettled is intentionally omitted because route-level invalidation (implemented in src/pageComponents/dashboard/index.tsx) automatically refreshes bookmark caches when navigating between categories, ensuring restored bookmarks appear when the user visits the destination category.
Learnt from: B-a-l-aj-i
Repo: timelessco/recollect PR: 706
File: src/pages/api/bookmark/add-url-screenshot.ts:180-189
Timestamp: 2026-01-27T08:57:35.427Z
Learning: In src/pages/api/bookmark/add-url-screenshot.ts, when additionalImages or additionalVideos collection fails, empty arrays are intentionally used instead of preserving existing meta_data values because the URLs returned from the screenshot API (allImages and allVideos) are temporary and expire after a few hours or 1-2 days. Keeping expired URLs would lead to broken media in the UI, so empty arrays prevent UI inconsistency.
Learnt from: abhishekmg
Repo: timelessco/recollect PR: 623
File: src/utils/api/bookmark/remaining.ts:374-375
Timestamp: 2025-11-26T10:14:58.025Z
Learning: In src/utils/api/bookmark/remaining.ts, the updateBookmarkWithRemainingData function intentionally matches only on id (not id + user_id) because the bookmark id is unique and authorization is verified at the API handler level before calling this utility function.
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2026-01-28T05:56:09.575Z
Learning: Applies to **/*.{ts,tsx} : Always include caption tracks for audio and video elements
Learnt from: navin-moorthy
Repo: timelessco/recollect PR: 694
File: src/components/lightbox/AddToCollectionDropdown.tsx:163-167
Timestamp: 2025-12-16T11:52:03.069Z
Learning: When reviewing code for react-aria-components usage, check for nightly build API changes in Select: replace selectedKeys/onSelectionChange with value/onChange. For single-select, value should be a scalar representing the item ID; for multi-select, value should be an array of item IDs and onChange should receive the updated array. Update any references in src/components/...tsx files accordingly (e.g., AddToCollectionDropdown.tsx) to use value and onChange with the appropriate types, and remove any remaining selectedKeys or onSelectionChange usage.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 732
File: src/components/toggleDarkMode.tsx:5-5
Timestamp: 2026-01-08T07:01:43.857Z
Learning: Within the repository where the settings modal is built using Ariakit Dialog (src/components/modal.tsx), prefer using Ariakit components (e.g., AriaDropdown) inside the modal to maintain accessibility, consistent styling, and reduce custom wiring. This guidance applies to TSX files under src/components that render modals or settings dialogs.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 722
File: src/pageComponents/settings/import.tsx:114-121
Timestamp: 2026-01-22T06:37:54.068Z
Learning: When reviewing React components that use React Query mutations (e.g., useImportBookmarksMutation), errors from the API or network are handled inside the mutation hook. Do not wrap the mutation call in a broad component-level try/catch for API errors; instead, handle only local, pre-mutation errors (such as CSV parsing or client-side validation) at the call site. Ensure the UI responds to mutation state (isLoading, isError, error) and presents user-friendly messages for mutation failures provided by the hook.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 747
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:40-41
Timestamp: 2026-02-04T05:04:02.699Z
Learning: Tailwind CSS v4 arbitrary spacing values are supported. In this repository (Tailwind v4.1.18), numeric spacing classes like -ml-0.25 or mt-21 are valid and map to N * 0.25rem (i.e., -ml-0.25 => -0.0625rem, mt-21 => 5.25rem). When reviewing TSX files, accept or apply these numeric spacing utilities as needed, ensuring the chosen values align with the design scale and responsive behavior.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 806
File: src/components/ui/recollect/tooltip.tsx:1-1
Timestamp: 2026-02-25T08:14:22.185Z
Learning: In the timelessco/recollect repo, enforce kebab-case for component file names across all .ts and .tsx files. For example, tooltip.tsx and scroll-area.tsx should be used; avoid camelCase or PascalCase such as Tooltip.tsx or ScrollArea.tsx. This improves consistency and import readability. Apply to all files matching the pattern **/*.{ts,tsx}.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 810
File: src/pageComponents/dashboard/share/share-public-switch.tsx:39-58
Timestamp: 2026-02-26T07:28:41.161Z
Learning: In the timelessco/recollect repo, when a Base UI Switch is rendered inside a menu/dropdown context (e.g., AriaDropdown), the onCheckedChange handler may not fire due to the menu's click handler closing the dropdown. Actionable workarounds: (1) use onClick instead of onCheckedChange, (2) set closeOnClick={false} to prevent the menu from closing on click, (3) call event.stopPropagation() in the handler to prevent the menu's click handler from triggering closure. This guideline applies to all .tsx files containing Switch components rendered inside menu/dropdown contexts across the codebase.
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 812
File: src/pageComponents/dashboard/sidePane/sidePaneUserDropdown.tsx:26-27
Timestamp: 2026-02-27T09:46:21.070Z
Learning: When reviewing focused refactoring PRs (such as UI library migrations or component replacements), do not flag pre-existing code quality issues (such as missing error handling, performance concerns, or architectural problems) as blocking concerns if they fall outside the stated scope of the refactor. Keep reviews focused on the refactoring objectives. Suggestions for improvements to pre-existing code should be marked as non-blocking, optional, or deferred to separate follow-up issues. This maintains PR focus and prevents scope creep while still documenting areas for future improvement.
Contributor
Author
|
design for audio case spotify artist ui: spotify song ui: audio player: <img width="927" height="541" alt="CleanShot 2026-03-03 at 14 17 50" src="https://github.com/user-attachments/a audio placeholder: 
Audio Player — Manual Test ChecklistTest against: dev environment Setup
Test 1 — Audio file / direct link (card view)
Test 2 — Audio file in lightbox (waveform player)
Test 3 — Spotify link
Screen.Recording.2026-03-03.at.2.42.52.PM.mov |
… YouTube, and Spotify
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit