Skip to content

Conversation

@joshngu
Copy link
Collaborator

@joshngu joshngu commented Oct 28, 2025

uses wildcards and searchs across all tables. home button works. buggy image_urls.

k0src and others added 16 commits September 16, 2025 10:22
Created Home Page and its Components
Split the 'dev' script into 'dev:server' and 'dev:client' for better separation of concerns. Added named and color-prefixed output to concurrently for easier log identification.
Updated search logic to query songs, artists, albums, and playlists using new table structures and fields. Expanded search to include additional fields (e.g., genre for songs, bio for artists, created_by for albums and playlists). Added error handling for each search type and standardized default values for missing images and metadata.
Added console.log statements to the search route to log incoming query parameters and the number of matched results for songs, artists, albums, and playlists. This will help with debugging and monitoring search activity.
@joshngu joshngu requested a review from Copilot October 28, 2025 15:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a functional search feature that enables users to search across songs, artists, albums, and playlists. Key changes include adding search routes on the backend, creating a dedicated search results page on the frontend, and improving the development workflow with better error handling and configuration.

Key Changes:

  • Added wildcard search functionality across all content types (songs, artists, albums, playlists)
  • Implemented search results page with navigation from the top bar
  • Enhanced server startup with graceful error handling and optional Azure Blob Storage configuration

Reviewed Changes

Copilot reviewed 30 out of 58 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
server/src/routes/search.routes.ts New search endpoint implementing wildcard queries across all tables
client/src/pages/SearchResultsPage/SearchResultsPage.tsx New search results page displaying categorized results
client/src/api/search.api.ts API client for search functionality
server/src/server.ts Updated server startup with better error handling and added helmet security middleware
server/src/config/blobStorage.ts Made Azure Blob Storage optional for local development
server/src/config/database.ts Added transaction helper and exported pool instance
client/src/components/TopBar/topBar.tsx Added search functionality with navigation to results page
package.json Updated dev scripts with better naming and concurrency setup

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

? process.env.ALLOWED_ORIGINS.split(",")
: true
: process.env.CLIENT_URL || "http://localhost:5173",
: process.env.CLIENT_URL || ["http://localhost:5173", "http://localhost:5174", "http://localhost:5175", "http://localhost:5176", "http://localhost:5177", "http://localhost:5178", "http://localhost:5179", "http://localhost:5180"],
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The hardcoded array of localhost URLs with sequential ports is difficult to maintain and lacks clarity. Consider using a range-based approach or environment variable with comma-separated values to make this more flexible and maintainable.

Copilot uses AI. Check for mistakes.
Comment on lines 26 to +29
credentials: true,
methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"],
allowedHeaders: ["Content-Type", "Authorization"],
allowedHeaders: ["Content-Type", "Authorization", "X-Requested-With"],
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

CORS is configured with credentials: true but allows multiple origins. This can be a security risk if the origin validation isn't strict. Ensure that when credentials are enabled, the origin is explicitly validated rather than using true for all origins.

Copilot uses AI. Check for mistakes.
const limitNum = parseInt(limit as string, 10);
const offsetNum = parseInt(offset as string, 10);

const results: any = {};
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

Using any type defeats TypeScript's type safety. Define a proper interface for the search results structure to ensure type safety throughout the search endpoint.

Copilot uses AI. Check for mistakes.
console.log(`[search] songs matched=${songsResult.rowCount}`);
results.songs = songsResult.rows.map((row: any) => ({
...row,
image: row.image || "/PlayerBar/Mask group.png",
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The fallback image path '/PlayerBar/Mask group.png' is hardcoded and appears in multiple places (lines 39, 65, 90, 118). Consider defining this as a constant at the module level or in a configuration file to ensure consistency and easier updates.

Copilot uses AI. Check for mistakes.
results.songs = songsResult.rows.map((row: any) => ({
...row,
image: row.image || "/PlayerBar/Mask group.png",
artist: row.genre || "Unknown",
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

Using genre as a fallback for artist is semantically incorrect. Genre and artist are distinct attributes. This should either use an actual artist field from the database or be clearly documented as a temporary workaround.

Copilot uses AI. Check for mistakes.
"build:server": "cd server && npm run build",
"build:client": "cd client && npm run build",
"install-all": "npm install && cd client && npm install && cd ../server && npm install",
"install:all": "npm install && cd client && npm install --legacy-peer-deps && cd ../server && npm install",
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The --legacy-peer-deps flag suggests dependency conflicts in the client. This should be investigated and resolved rather than perpetually using a compatibility flag, as it may mask underlying issues or security vulnerabilities.

Suggested change
"install:all": "npm install && cd client && npm install --legacy-peer-deps && cd ../server && npm install",
"install:all": "npm install && cd client && npm install && cd ../server && npm install",

Copilot uses AI. Check for mistakes.
<section className={homeStyles.recentlyPlayedColumn}>
<div className={homeStyles.sectionHeader}>
<h2 className={homeStyles.sectionTitle}>Songs</h2>
<a href="#" className={homeStyles.viewMore}>View More</a>
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The 'View More' links use href=\"#\" which doesn't provide meaningful navigation and can cause accessibility issues. These should either be proper navigation links or button elements if they're meant to trigger actions.

Copilot uses AI. Check for mistakes.
// provided (e.g., http://localhost:8080), append "/api". Otherwise, default
// to the Vite dev proxy on "/api" so requests are proxied during development.
const resolvedBaseUrl = API_BASE_URL
? `${API_BASE_URL.replace(/\/$/, "")}/api`
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The logic assumes that if API_BASE_URL is provided, it needs '/api' appended. However, if API_BASE_URL already includes '/api', this will result in '/api/api'. Add validation to check if the path already ends with '/api' before appending.

Suggested change
? `${API_BASE_URL.replace(/\/$/, "")}/api`
? (() => {
const base = API_BASE_URL.replace(/\/$/, "");
return base.endsWith("/api") ? base : `${base}/api`;
})()

Copilot uses AI. Check for mistakes.
<h2 className={styles.featuredTitle}>{title}</h2>
<p className={styles.featuredDescription}>{description}</p>
<div className={styles.featuredMeta}>
<img className={styles.metaIcon} />
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The img element is missing required src and alt attributes, which violates HTML standards and accessibility requirements. Either provide these attributes or remove the element if it's not being used.

Suggested change
<img className={styles.metaIcon} />
{/* Replace 'icon-heart.png' with the actual path to your icon asset */}
<img src="/icons/icon-heart.png" alt="Likes" className={styles.metaIcon} />

Copilot uses AI. Check for mistakes.
@joshngu joshngu requested a review from Copilot October 29, 2025 16:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 30 out of 58 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

server/src/routes/search.routes.ts:2

  • Unused import getBlobUrl.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -1,5 +1,5 @@
import express, { Request, Response } from "express";
import { SongRepository } from "@repositories";
import { SongRepository } from "repos";
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

The import path 'repos' is not aligned with the TypeScript path mapping configuration. The tsconfig.json defines '@repositories' as the path alias, but this import uses a bare 'repos' specifier. This should be either 'repos/index.js' with a relative path (e.g., '../repos') or use the configured alias '@repositories' for consistency.

Copilot uses AI. Check for mistakes.
throw new Error("Missing required Postgres environment variables.");
}

// ssl: process.env.NODE_ENV === 'production' ? { rejectUnauthorized: true } : { rejectUnauthorized: false }, not working idek fix later
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'idek' to 'I don't even know'.

Copilot uses AI. Check for mistakes.
throw new Error("Missing required Postgres environment variables.");
}

// ssl: process.env.NODE_ENV === 'production' ? { rejectUnauthorized: true } : { rejectUnauthorized: false }, not working idek fix later
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

This commented-out SSL configuration with an incomplete explanation ('not working idek fix later') should either be properly documented with the specific issue encountered or removed. If SSL is intentionally disabled for a reason, that reason should be clearly documented.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,137 @@
import express, { Request, Response } from "express";
import { getBlobUrl } from "../config/blobStorage.js";
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

The import of getBlobUrl is unused in this file. The search queries return raw database values without transforming blob URLs, which means image URLs won't be properly resolved to SAS token URLs when Azure Blob Storage is configured.

Copilot uses AI. Check for mistakes.
const limitNum = parseInt(limit as string, 10);
const offsetNum = parseInt(offset as string, 10);

const results: any = {};
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Using the 'any' type removes type safety. Consider defining a proper TypeScript interface for the search results object that matches the SearchResponse interface expected by the client (with optional properties for songs, artists, albums, and playlists arrays).

Copilot uses AI. Check for mistakes.

=== Roles

+ *Unauthenticated User* - Can browse the site, view songs, artist profiles, and playlists. These users have limited access and cannot interact with the site beyond causal browsing.
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'causal' to 'casual'.

Copilot uses AI. Check for mistakes.

== Data Entry Forms

Data entry forms provide a method for users and artists generate content for the platform.
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Missing 'to' before 'generate'. Should read 'users and artists to generate content'.

Copilot uses AI. Check for mistakes.
Comment on lines +423 to +425
+ *Statistics/Insights*
- Site-wide statistics page
- Artist's 10 ten most played/liked songs
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Corrected duplication of 'ten' to just 'top ten'.

Copilot uses AI. Check for mistakes.
Improved environment variable loading and CORS handling in server startup, made database connection checks non-blocking, and enhanced health check endpoint. Refactored search route for safer queries and simplified logic. Added several new dependencies to client/package.json for UI and audio features.
Updated package-lock.json to include @mui/material, @emotion/react, @emotion/styled, @mui/x-charts, @wavesurfer/react, react-spinners, uuid, and wavesurfer.js, along with their dependencies. This prepares the client for new UI and audio features.
Added Inter font via Google Fonts in index.html and applied it to key UI elements for consistent typography. Updated CSS in SongCard, HomePage, and SearchResultsPage to use Inter and semi-bold weights. Removed unused 'View More' links from SearchResultsPage and styled the loading text.
Introduces the EntityItem React component and its CSS module for displaying songs, playlists, albums, or artists with action buttons and navigation. This component supports both small and wide layouts, handles play and queue actions, and displays relevant entity information.
Deleted FeaturedSection, NewSongSection, RecentSection components and their associated CSS module. This likely reflects a refactor or redesign of the HomePage structure.
Added new styles for EntityItem and updated PlayerBar styles for better font consistency. Changed song list rendering in SearchResultsPage to use song.id as the key instead of the array index. Added clarifying comments in search.routes.ts regarding blob URL resolution.
Introduces new components for the artist page, including ArtistBanner, ArtistAbout, ArtistActions, ArtistPlaylists, and RelatedArtists, along with their styles. Removes old PlayerBar, SideBar, and TopBar components, and updates routing to support artist pages. Refactors layout structure with AppLayout and MainLayout, adds utility and test components, and updates API types for artist and search functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants