Feature/iw 43 create song view by WWarzecha · Pull Request #8 · Vemtor/karaoke

WWarzecha · 2025-05-08T18:16:39Z

No description provided.

Galaktikkon

Suggestions after live-review:

group over-segmented types into one file
change design of the control-panel
remove unnecessary console.logs

…te-song-view

Galaktikkon

Good job migrating to NativeWind. Please consider my architectural suggestions and investigate the possible code-smells that caught my eye. Try to clean the classes - especially colors. Propose a fix for the API route placement, so that it's easy to translate to production.

Galaktikkon · 2025-05-19T17:43:12Z

frontend/components/karaoke/KaraokeText.tsx

+    <View className="w-full h-full flex justify-center items-center bg-[#191414]">
+      <View className="relative w-full flex justify-center items-center">
+        <Text className="text-[22px] font-roboto-mono text-white text-center my-2 z-0">
+          {songLines.previousLine}
+        </Text>
+        <View className="absolute w-full h-full top-0 left-0 z-1 bg-gradient-to-b from-[rgba(25,20,20,1)] from-[75%_rgba(25,20,20,0.5)] to-[rgba(0,0,0,0)]" />
+      </View>
+      <Text className="text-[22px] font-roboto-mono text-[#95E558] text-center my-2">
+        {songLines.currentLine}
+      </Text>
+      <View className="relative w-full flex justify-center items-center">
+        <Text className="text-[22px] font-roboto-mono text-white text-center my-2 z-0">
+          {songLines.nextLine}
+        </Text>
+        <View className="absolute w-full h-full top-0 left-0 z-1 bg-gradient-to-t from-[rgba(25,20,20,1)] from-[75%_rgba(25,20,20,0.5)] to-[rgba(0,0,0,0)]" />
+      </View>
+    </View>
+  );
+};


These colors should be added to the project if they are not already present.

Galaktikkon · 2025-05-19T17:50:48Z

frontend/app/(tabs)/karaoke.tsx

+const KaraokeScreen = () => {
+  return (
+    <SafeAreaView className="h-full">
+      <SongViewText />
+      <ControlPanel />
+    </SafeAreaView>
+  );
+};


Use ViewLayout component for uniform view layout.

Galaktikkon · 2025-05-19T17:53:57Z

frontend/components/karaoke/ControlPanel.tsx

+        <TouchableOpacity className="items-center" onPress={togglePanel}>
+          {isOpen ? <ChevronDown /> : <ChevronUp />}
+        </TouchableOpacity>


Clickable area covered by the button is too small. It should be a bit bigger than the icon itself (eg. 30x30 or 40x40).

Galaktikkon · 2025-05-19T17:57:48Z

frontend/components/karaoke/ControlPanel.tsx

+  });
+
+  return (
+    <View className="bg-gray-300 py-2 border-t border-gray-400 justify-around items-center flex flex-col absolute w-full bottom-0">


This gray color stands out too much. Also there is no line between the NavigationBar and the ControlPanel which just looks bad.

Galaktikkon · 2025-05-19T18:00:24Z

frontend/components/karaoke/KaraokeText.tsx

+        <View className="absolute w-full h-full top-0 left-0 z-1 bg-gradient-to-b from-[rgba(25,20,20,1)] from-[75%_rgba(25,20,20,0.5)] to-[rgba(0,0,0,0)]" />
+      </View>
+      <Text className="text-[22px] font-roboto-mono text-[#95E558] text-center my-2">
+        {songLines.currentLine}
+      </Text>
+      <View className="relative w-full flex justify-center items-center">
+        <Text className="text-[22px] font-roboto-mono text-white text-center my-2 z-0">
+          {songLines.nextLine}
+        </Text>
+        <View className="absolute w-full h-full top-0 left-0 z-1 bg-gradient-to-t from-[rgba(25,20,20,1)] from-[75%_rgba(25,20,20,0.5)] to-[rgba(0,0,0,0)]" />


Fix the colors (they should be added to the project scope). Also you use from-* two times which is unnecessary and may cause unexpected behaviour.

Galaktikkon · 2025-05-19T18:04:42Z

frontend/components/karaoke/Controls.tsx

+    const { playNextSong, playPauseSong, playPreviousSong, isPlaying } = useTrackPlayer();
+    return (
+    <View className="flex flex-row justify-around items-center w-full">
+        <TouchableOpacity className="items-center" onPress={playPreviousSong}>
+            <SkipBack size={32} color="black" />
+        </TouchableOpacity>
+        <TouchableOpacity className="items-center" onPress={playPauseSong}>
+            {isPlaying ? <Pause size={32} color="black" /> : <Play size={32} color="black" />}
+        </TouchableOpacity>
+        <TouchableOpacity className="items-center" onPress={playNextSong}>
+            <SkipForward size={32} color="black" />
+        </TouchableOpacity>
+    </View>


These could also be a dedicated component (like IconButton). Again try to make the button clickable area bigger for better UX.

Galaktikkon · 2025-05-19T18:12:11Z

frontend/context/trackPlayerContext.tsx

+  const playPauseSong = async () => {
+    if (!isTrackPlayerReady) {
+      console.warn('TrackPlayer is not ready yet!');
+      return;
+    }
+    const state = await TrackPlayer.getState();
+    if (state === State.Playing) {
+      TrackPlayer.pause();
+    } else {
+      TrackPlayer.play();
+    }
+  };


Maybe toggleSong would be a bettter name?

Galaktikkon · 2025-05-19T18:20:27Z

frontend/context/trackPlayerContext.tsx

+      const fetchedSongText = await fetchSongLyrics(youtubeUrl);
+      const fetchedAudioSplitterResponse = await splitAudio(youtubeUrl);
+      const songUrl = 'http://localhost:8080' + fetchedAudioSplitterResponse.instrumentsPath;
+
+      track.url = songUrl; // provide url of the file location to the track
+      track.songText = fetchedSongText; // provide song text to the track
+


This seems like a code-smell to me. I do not know the backend architecture, so please explain the songUrl purpose. Does the backend expose a specific route for this? What for? Also the root route should be saved in some file (not hardcoded), so that it can be easily swapped for the production.

Galaktikkon · 2025-05-19T18:21:54Z

frontend/context/trackPlayerContext.tsx

+      const currentSegmentIndex = songText.segments.findIndex(
+        (segment: SongSegment) =>
+          currentTime >= segment.start - 0.5 && currentTime < segment.end - 0.5,
+      );
+
+      // initial text
+      if (currentTime < 0.1 && currentSegmentIndex === -1) {
+        setSongLines({
+          previousSegmentIndex: -1,
+          previousLine: '',
+          currentLine: '',
+          nextLine: songText.segments[0]?.text,
+        });
+      // normal case
+      } else if (currentSegmentIndex !== -1) {
+        setSongLines({
+          previousSegmentIndex: currentSegmentIndex,
+          previousLine: songText.segments[currentSegmentIndex - 1]?.text || '',
+          currentLine: songText.segments[currentSegmentIndex]?.text || '',
+          nextLine: songText.segments[currentSegmentIndex + 1]?.text || '',
+        });
+      // between lines (silence) case
+      } else {
+        setSongLines({
+          previousSegmentIndex: songLines.previousSegmentIndex,
+          previousLine: songText.segments[songLines.previousSegmentIndex]?.text || '',
+          currentLine: ' ',
+          nextLine: songText.segments[songLines.previousSegmentIndex + 1]?.text || '',
+        });
+      }
+    };


This logic is not straightforward. (e.g. the boolean comparison that transfers into an index - is it really an index? not a flag or state?) Please try to simplify it so that it follows KISS.

frontend/services/backendApi.ts

nice review, thanks for it but Wiktor done gr8 job

WWarzecha added 2 commits May 8, 2025 20:11

Implement song view

0ffe86c

hotfix: add cors to backend

7900dc6

WWarzecha requested a review from Galaktikkon May 8, 2025 18:16

WWarzecha added 3 commits May 13, 2025 17:06

code refactor

6968c9c

linter changes

15d52c0

fix CorsConfig for Spotless

5593591

Galaktikkon requested changes May 17, 2025

View reviewed changes

WWarzecha added 3 commits May 18, 2025 23:48

Merge remote-tracking branch 'origin/develop' into feature/IW-43-Crea…

db6c2b0

…te-song-view

code refactor

3d99b6d

remove unnecessary comments

f3276d4

Galaktikkon previously requested changes May 19, 2025

View reviewed changes

code fixes

99d4348

WWarzecha requested a review from Vemtor May 19, 2025 19:00

Vemtor approved these changes May 19, 2025

View reviewed changes

WWarzecha merged commit d5870af into develop May 19, 2025

Galaktikkon deleted the feature/IW-43-Create-song-view branch May 29, 2025 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/iw 43 create song view#8

Feature/iw 43 create song view#8
WWarzecha merged 9 commits intodevelopfrom
feature/IW-43-Create-song-view

WWarzecha commented May 8, 2025

Uh oh!

Galaktikkon left a comment

Uh oh!

Galaktikkon left a comment

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Galaktikkon May 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

WWarzecha commented May 8, 2025

Uh oh!

Galaktikkon left a comment

Choose a reason for hiding this comment

Uh oh!

Galaktikkon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants