Skip to content

feature-itunes-ticket-NA #207

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
SiddOnKeys wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feature-itunes-ticket-NA #207

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions app/components/SongCard/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import React, { useState, useRef } from 'react';
import PropTypes from 'prop-types';
import { Card, CardContent, CardMedia, Typography, IconButton, Box } from '@mui/material';
import { PlayArrow, Pause } from '@mui/icons-material';
import styled from '@emotion/styled';

const StyledCard = styled(Card)`
position: relative;
width: 100%;
transition: transform 0.2s ease-in-out;
cursor: pointer;

&:hover {
transform: translateY(-4px);
}
`;

const PlayButton = styled(IconButton)`
position: absolute;
right: 8px;
bottom: 8px;
background-color: rgba(147, 51, 234, 0.9);

&:hover {
background-color: rgba(147, 51, 234, 1);
}
`;

const ArtistText = styled(Typography)`
color: ${({ theme }) => theme.palette.text.secondary};
font-size: 0.875rem;
`;

function SongCard({ track }) {
const [isPlaying, setIsPlaying] = useState(false);
const audioRef = useRef(null);

const handlePlayPause = (e) => {
e.stopPropagation();
if (!audioRef.current) {
audioRef.current = new Audio(track.previewUrl);
audioRef.current.addEventListener('ended', () => setIsPlaying(false));
}

if (isPlaying) {
audioRef.current.pause();
} else {
audioRef.current.play();
}
setIsPlaying(!isPlaying);
Comment on lines +45 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot May 30, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for audio playback.

The audio.play() method returns a Promise that can be rejected if playback fails. This should be handled to prevent unhandled promise rejections.

    if (isPlaying) {
      audioRef.current.pause();
    } else {
-      audioRef.current.play();
+      audioRef.current.play().catch((error) => {
+        console.error('Audio playback failed:', error);
+        setIsPlaying(false);
+      });
    }
    setIsPlaying(!isPlaying);
📝 Committable suggestion

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

Suggested change
if (isPlaying) {
audioRef.current.pause();
} else {
audioRef.current.play();
}
setIsPlaying(!isPlaying);
if (isPlaying) {
audioRef.current.pause();
} else {
audioRef.current.play().catch((error) => {
console.error('Audio playback failed:', error);
setIsPlaying(false);
});
}
setIsPlaying(!isPlaying);
🤖 Prompt for AI Agents 
In app/components/SongCard/index.js around lines 45 to 50, the audio.play() call
returns a Promise that may reject if playback fails, but currently there is no
error handling. Modify the code to handle the Promise returned by
audioRef.current.play() by adding a .catch() block to catch and handle any
playback errors, preventing unhandled promise rejections.

};

React.useEffect(() => {
return () => {
if (audioRef.current) {
audioRef.current.pause();
audioRef.current.removeEventListener('ended', () => setIsPlaying(false));
Copy link

@gemini-code-assist gemini-code-assist bot May 30, 2025

Choose a reason for hiding this comment

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

high

The event listener for 'ended' added in handlePlayPause (line 42) uses an inline anonymous function () => setIsPlaying(false). The removeEventListener call here in the useEffect cleanup hook also uses a new anonymous function instance.

Because these are different function instances, removeEventListener will not be able to find and remove the listener that was originally added. This will lead to a memory leak, as listeners will accumulate every time handlePlayPause is called and a new audio element is potentially created (though current logic reuses it if audioRef.current exists).

Could you define the event handler function in a stable way (e.g., using useCallback or defining it outside handlePlayPause but within the component's scope if it depends on component state/props) and use that same reference for both addEventListener and removeEventListener?

        // Define this handler in a stable way, e.g., using useCallback
        // const handleAudioEnded = useCallback(() => setIsPlaying(false), []);
        // audioRef.current.removeEventListener('ended', handleAudioEnded);
        // For a direct fix here, assuming you'll refactor handlePlayPause too:
        // This line needs to reference the *exact same function instance* as addEventListener
        // A proper fix involves refactoring how the listener is defined and passed.
        // Example (conceptual, full change involves handlePlayPause):
        // const onAudioEnd = audioRef.current._onAudioEndHandler; // Assuming you store it
        // if (onAudioEnd) audioRef.current.removeEventListener('ended', onAudioEnd);
        audioRef.current.removeEventListener('ended', () => setIsPlaying(false)); // This line is problematic

audioRef.current = null;
}
};
}, []);
Comment on lines +53 to +61
Copy link
Contributor

@coderabbitai coderabbitai bot May 30, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix audio cleanup in useEffect.

The current cleanup function has a bug - the event listener removal won't work because it creates a new function reference instead of removing the original listener.

function SongCard({ track }) {
  const [isPlaying, setIsPlaying] = useState(false);
  const audioRef = useRef(null);
+  const endedListenerRef = useRef(null);

  const handlePlayPause = (e) => {
    e.stopPropagation();
    if (!audioRef.current) {
      audioRef.current = new Audio(track.previewUrl);
-      audioRef.current.addEventListener('ended', () => setIsPlaying(false));
+      endedListenerRef.current = () => setIsPlaying(false);
+      audioRef.current.addEventListener('ended', endedListenerRef.current);
    }
    // ... rest of function
  };

  React.useEffect(() => {
    return () => {
      if (audioRef.current) {
        audioRef.current.pause();
-        audioRef.current.removeEventListener('ended', () => setIsPlaying(false));
+        if (endedListenerRef.current) {
+          audioRef.current.removeEventListener('ended', endedListenerRef.current);
+        }
        audioRef.current = null;
      }
    };
  }, []);
📝 Committable suggestion

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

Suggested change
React.useEffect(() => {
return () => {
if (audioRef.current) {
audioRef.current.pause();
audioRef.current.removeEventListener('ended', () => setIsPlaying(false));
audioRef.current = null;
}
};
}, []);
function SongCard({ track }) {
const [isPlaying, setIsPlaying] = useState(false);
const audioRef = useRef(null);
const endedListenerRef = useRef(null);
const handlePlayPause = (e) => {
e.stopPropagation();
if (!audioRef.current) {
audioRef.current = new Audio(track.previewUrl);
endedListenerRef.current = () => setIsPlaying(false);
audioRef.current.addEventListener('ended', endedListenerRef.current);
}
// ... rest of function
};
React.useEffect(() => {
return () => {
if (audioRef.current) {
audioRef.current.pause();
if (endedListenerRef.current) {
audioRef.current.removeEventListener('ended', endedListenerRef.current);
}
audioRef.current = null;
}
};
}, []);
// ... rest of component
}
🧰 Tools
🪛 ESLint

[error] 58-58: No object mutation allowed.

(immutable/no-mutation)

🤖 Prompt for AI Agents 
In app/components/SongCard/index.js around lines 53 to 61, the cleanup function
in useEffect incorrectly removes the 'ended' event listener by passing a new
anonymous function, which does not match the original listener reference. To fix
this, store the event handler function in a variable when adding the listener,
then use the same variable to remove the listener in the cleanup function,
ensuring proper removal of the event listener.


return (
<StyledCard>
<CardMedia
component="img"
height="200"
image={track.artworkUrl100.replace('100x100bb', '400x400bb')}
alt={track.trackName}
/>
<CardContent>
<Typography variant="h6" noWrap>
{track.trackName}
</Typography>
<ArtistText variant="body2" noWrap>
{track.artistName}
</ArtistText>
<Box sx={{ mt: 1 }}>
<Typography variant="caption" color="text.secondary">
{track.primaryGenreName}
</Typography>
</Box>
</CardContent>
<PlayButton aria-label={isPlaying ? 'pause' : 'play'} onClick={handlePlayPause} size="medium">
{isPlaying ? <Pause /> : <PlayArrow />}
</PlayButton>
</StyledCard>
);
}

SongCard.propTypes = {
track: PropTypes.shape({
trackName: PropTypes.string.isRequired,
artistName: PropTypes.string.isRequired,
artworkUrl100: PropTypes.string.isRequired,
previewUrl: PropTypes.string.isRequired,
primaryGenreName: PropTypes.string.isRequired
}).isRequired
};

export default SongCard;
111 changes: 83 additions & 28 deletions app/containers/App/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import { Provider } from 'react-redux';
import { CssBaseline, Container } from '@mui/material';
import { ThemeProvider as MUIThemeProvider, createTheme, StyledEngineProvider } from '@mui/material/styles';
import { Global } from '@emotion/react';
import styled from '@emotion/styled';
import { routeConfig } from '@app/routeConfig';
import globalStyles from '@app/global-styles';
import { Header } from '@components/Header';
import { ScrollToTop } from '@components/ScrollToTop';
import { For } from '@components/For';
import { If } from '@app/components/If';
Expand All @@ -29,21 +29,72 @@ import { translationMessages } from '@app/i18n';
import history from '@utils/history';
import { SCREEN_BREAK_POINTS } from '@utils/constants';
import configureStore from '@app/configureStore';
import { colors } from '@themes';

const TRANSPARENT_VIOLET_BG = 'rgba(30, 27, 75, 0.5)';

const BlurredBackground = styled.div`
position: fixed;
top: 0;
left: 0;
right: 0;
bottom: 0;
background: linear-gradient(135deg, rgba(76, 0, 130, 0.4) 0%, rgba(147, 51, 234, 0.4) 100%);
backdrop-filter: blur(10px);
z-index: -1;
`;

const StyledContainer = styled(Container)`
min-height: 100vh;
position: relative;
z-index: 1;
`;

export const theme = createTheme({
palette: {
mode: 'dark',
primary: {
main: colors.primary
main: '#9333EA', // Bright violet
light: '#A855F7',
dark: '#7E22CE'
},
secondary: {
main: colors.secondary
main: '#2D1B69', // Dark violet
light: '#3730A3',
dark: '#1E1B4B'
},
background: {
default: '#0F172A', // Dark blue-gray
paper: TRANSPARENT_VIOLET_BG // Semi-transparent dark violet
},
text: {
primary: '#F8FAFC',
secondary: '#CBD5E1'
}
},
components: {
MuiPaper: {
styleOverrides: {
root: {
backgroundImage: 'none',
backgroundColor: TRANSPARENT_VIOLET_BG,
backdropFilter: 'blur(8px)'
}
}
},
MuiCard: {
styleOverrides: {
root: {
backgroundColor: TRANSPARENT_VIOLET_BG,
backdropFilter: 'blur(8px)'
}
}
}
},
breakpoints: {
values: SCREEN_BREAK_POINTS
}
});

/**
* App component that sets up the application with routing, theme, and language support.
* It also handles redirect logic based on the query parameters in the URL.
Expand Down Expand Up @@ -79,30 +130,32 @@ export function App() {
<MUIThemeProvider theme={theme}>
<CssBaseline />
<Global styles={globalStyles} />
<Header />
<Container>
<For
ParentComponent={(props) => <Switch {...props} />}
of={map(Object.keys(routeConfig))}
renderItem={(routeKey, index) => {
const Component = routeConfig[routeKey].component;
return (
<Route
exact={routeConfig[routeKey].exact}
key={index}
path={routeConfig[routeKey].route}
render={(props) => {
const updatedProps = {
...props,
...routeConfig[routeKey].props
};
return <Component {...updatedProps} />;
}}
/>
);
}}
/>
</Container>
<>
<BlurredBackground />
<StyledContainer>
<For
ParentComponent={(props) => <Switch {...props} />}
of={map(Object.keys(routeConfig))}
renderItem={(routeKey, index) => {
const Component = routeConfig[routeKey].component;
return (
<Route
exact={routeConfig[routeKey].exact}
key={index}
path={routeConfig[routeKey].route}
render={(props) => {
const updatedProps = {
...props,
...routeConfig[routeKey].props
};
return <Component {...updatedProps} />;
}}
/>
);
}}
/>
</StyledContainer>
</>
</MUIThemeProvider>
</StyledEngineProvider>
</ConnectedLanguageProvider>
Expand All @@ -114,8 +167,10 @@ export function App() {
</If>
);
}

App.propTypes = {
location: PropTypes.object,
history: PropTypes.object
};

export default App;
39 changes: 39 additions & 0 deletions app/containers/ITunesSearch/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { SEARCH_TRACKS, SEARCH_TRACKS_SUCCESS, SEARCH_TRACKS_ERROR, CLEAR_TRACKS } from './constants';

/**
* Search for tracks action
* @param {string} query - The search query
* @returns {object} An action object with a type of SEARCH_TRACKS
*/
export const searchTracks = (query) => ({
type: SEARCH_TRACKS,
query
});

/**
* Dispatched when the search is successful
* @param {array} tracks - The track results
* @returns {object} An action object with a type of SEARCH_TRACKS_SUCCESS
*/
export const searchTracksSuccess = (tracks) => ({
type: SEARCH_TRACKS_SUCCESS,
tracks
});

/**
* Dispatched when the search fails
* @param {object} error - The error object
* @returns {object} An action object with a type of SEARCH_TRACKS_ERROR
*/
export const searchTracksError = (error) => ({
type: SEARCH_TRACKS_ERROR,
error
});

/**
* Clear the tracks from store
* @returns {object} An action object with a type of CLEAR_TRACKS
*/
export const clearTracks = () => ({
type: CLEAR_TRACKS
});
4 changes: 4 additions & 0 deletions app/containers/ITunesSearch/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const SEARCH_TRACKS = 'app/ITunesSearch/SEARCH_TRACKS';
export const SEARCH_TRACKS_SUCCESS = 'app/ITunesSearch/SEARCH_TRACKS_SUCCESS';
export const SEARCH_TRACKS_ERROR = 'app/ITunesSearch/SEARCH_TRACKS_ERROR';
export const CLEAR_TRACKS = 'app/ITunesSearch/CLEAR_TRACKS';
Loading
Loading