Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to be a work-in-progress branch with multiple critical bugs and incomplete implementations. The changes attempt to refactor visualization components, update API endpoints, and modify the event management system, but introduce numerous breaking changes and logic errors.
Key Changes:
- Replaced visualization card implementation to use react-chartjs-2 exclusively (removing support for multiple chart types)
- Modified event count API endpoint and dashboard integration
- Removed the entire "Add User" form functionality, replacing it with placeholder content
- Refactored event form submission from HTTP to WebSocket-based communication
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added chart.js dependencies, removed ESLint from devDependencies, updated update package version |
| frontend/src/pages/eventOverview.js | Restructured visualization cards layout, removed imports, introduced duplicate card titles and indentation issues |
| frontend/src/pages/dashboard.js | Added event count fetching with polling, modified registration chart data, introduced API/response field mismatches |
| frontend/src/pages/addUser.js | Completely gutted form functionality, replaced with placeholder "Events" content |
| frontend/src/pages/addEvent.js | Changed form submission from HTTP to WebSocket, consolidated coordinator state causing all dropdowns to sync incorrectly |
| frontend/src/components/visualizationCard.js | Breaking rewrite to only support Bar charts, removed theme support, hardcoded white background |
| frontend/src/components/metricCard.js | Changed all text to white color causing accessibility issues, added bgColor prop |
| frontend/package.json | Removed circular frontend: "file:" dependency |
| backend/server.js | Reorganized imports, changed EventRoutes path, removed UserAdd routes, fixed case sensitivity bug in header check |
| backend/routes/event.js | New file with /events-count endpoint that has path and response field mismatches with frontend |
| backend/package.json | Removed circular backend: "file:" dependency |
| backend/models/events.js | Deleted entire events model file |
Files not reviewed (2)
- backend/package-lock.json: Language not supported
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Grid item xs={12} md={6}> | ||
| <FormControl fullWidth required> | ||
| <InputLabel sx={{ color: theme.palette.mode === "dark" ? "#fff" : "#000" }}>Coordinator 1</InputLabel> | ||
| <Select value={coordinator1} onChange={(e) => setCoordinator1(e.target.value)} sx={inputStyles}> | ||
| <InputLabel>Coordinator 1</InputLabel> | ||
| <Select | ||
| value={coordinator} | ||
| onChange={(e) => setCoordinator(e.target.value)} | ||
| label="Coordinator" | ||
| > | ||
| {coordinators.map((coord, index) => ( | ||
| <MenuItem key={index} value={coord}>{coord}</MenuItem> | ||
| <MenuItem key={index} value={coord}> | ||
| {coord} | ||
| </MenuItem> | ||
| ))} | ||
| </Select> | ||
| </FormControl> | ||
| </Grid> | ||
|
|
||
| {/* Faculty Coordinator 1 */} | ||
| <Grid item xs={12} md={6}> | ||
| <FormControl fullWidth required> | ||
| <InputLabel sx={{ color: theme.palette.mode === "dark" ? "#fff" : "#000" }}>Faculty Coordinator 1</InputLabel> | ||
| <Select value={facultyCoordinator1} onChange={(e) => setFacultyCoordinator1(e.target.value)} sx={inputStyles}> | ||
| <InputLabel>Faculty Coordinator 1</InputLabel> | ||
| <Select | ||
| value={coordinator} | ||
| onChange={(e) => setCoordinator(e.target.value)} | ||
| label="Coordinator" | ||
| > | ||
| {coordinators.map((coord, index) => ( | ||
| <MenuItem key={index} value={coord}>{coord}</MenuItem> | ||
| <MenuItem key={index} value={coord}> | ||
| {coord} | ||
| </MenuItem> | ||
| ))} | ||
| </Select> | ||
| </FormControl> | ||
| </Grid> | ||
|
|
||
| {/* Coordinator 2 */} | ||
| <Grid item xs={12} md={6}> | ||
| <FormControl fullWidth> | ||
| <InputLabel sx={{ color: theme.palette.mode === "dark" ? "#fff" : "#000" }}>Coordinator 2</InputLabel> | ||
| <Select value={coordinator2} onChange={(e) => setCoordinator2(e.target.value)} sx={inputStyles}> | ||
| <FormControl fullWidth > | ||
| <InputLabel>Coordinator 2</InputLabel> | ||
| <Select | ||
| value={coordinator} | ||
| onChange={(e) => setCoordinator(e.target.value)} | ||
| label="Coordinator" | ||
| > | ||
| {coordinators.map((coord, index) => ( | ||
| <MenuItem key={index} value={coord}>{coord}</MenuItem> | ||
| <MenuItem key={index} value={coord}> | ||
| {coord} | ||
| </MenuItem> | ||
| ))} | ||
| </Select> | ||
| </FormControl> | ||
| </Grid> | ||
|
|
||
| {/* Faculty Coordinator 2 */} | ||
| <Grid item xs={12} md={6}> | ||
| <FormControl fullWidth> | ||
| <InputLabel sx={{ color: theme.palette.mode === "dark" ? "#fff" : "#000" }}>Faculty Coordinator 2</InputLabel> | ||
| <Select value={facultyCoordinator2} onChange={(e) => setFacultyCoordinator2(e.target.value)} sx={inputStyles}> | ||
| <FormControl fullWidth > | ||
| <InputLabel> Faculty Coordinator 2</InputLabel> | ||
| <Select | ||
| value={coordinator} | ||
| onChange={(e) => setCoordinator(e.target.value)} | ||
| label="Coordinator" | ||
| > | ||
| {coordinators.map((coord, index) => ( | ||
| <MenuItem key={index} value={coord}>{coord}</MenuItem> | ||
| <MenuItem key={index} value={coord}> | ||
| {coord} | ||
| </MenuItem> | ||
| ))} | ||
| </Select> | ||
| </FormControl> |
There was a problem hiding this comment.
All four coordinator dropdowns (Coordinator 1, Faculty Coordinator 1, Coordinator 2, Faculty Coordinator 2) are bound to the same state variable coordinator and use the same onChange handler. This means selecting a value in any dropdown will update all of them simultaneously, making it impossible to select different coordinators. Each dropdown needs its own state variable.
| sx={{ | ||
| height, | ||
| width, | ||
| transition: "all 0.3s ease-in-out", | ||
| boxShadow: "0px 4px 8px rgba(32, 24, 24, 0.1)", | ||
| backgroundColor: "#ffffff", // Default white | ||
| "&:hover": { | ||
| boxShadow: "0px 8px 16px rgba(17, 15, 15, 0.2)", | ||
| transform: "scale(1.03)", | ||
| backgroundColor: "#f0f0f0", // Slightly darkened white | ||
| }, | ||
| }} |
There was a problem hiding this comment.
Hard-coded color values (#ffffff, #f0f0f0) in the card styling don't respect the Material-UI theme or dark mode settings. The original implementation used the darkMode prop to handle theming, but this has been removed. This will cause visual issues in dark mode.
| sx={{ | |
| height, | |
| width, | |
| transition: "all 0.3s ease-in-out", | |
| boxShadow: "0px 4px 8px rgba(32, 24, 24, 0.1)", | |
| backgroundColor: "#ffffff", // Default white | |
| "&:hover": { | |
| boxShadow: "0px 8px 16px rgba(17, 15, 15, 0.2)", | |
| transform: "scale(1.03)", | |
| backgroundColor: "#f0f0f0", // Slightly darkened white | |
| }, | |
| }} | |
| sx={(theme) => ({ | |
| height, | |
| width, | |
| transition: "all 0.3s ease-in-out", | |
| boxShadow: "0px 4px 8px rgba(32, 24, 24, 0.1)", | |
| backgroundColor: theme.palette.background.paper, // Theme-aware background | |
| "&:hover": { | |
| boxShadow: "0px 8px 16px rgba(17, 15, 15, 0.2)", | |
| transform: "scale(1.03)", | |
| backgroundColor: theme.palette.action.hover, // Theme-aware hover | |
| }, | |
| })} |
| <Typography variant="h6" sx={{ fontSize: "20px", fontWeight: "600", color: "#fff" }}> | ||
| {title} | ||
| </Typography> | ||
|
|
||
| {/* Venue or Numeric Value */} | ||
| <Typography variant="h5" sx={{ fontSize: "16px", fontWeight: "600", color: darkMode ? "#a9a9a9" : "#000" }}> | ||
| <Typography variant="h5" sx={{ fontSize: "16px", fontWeight: "600", color: "#fff" }}> | ||
| {isNumeric ? <CountUp end={Number(value)} duration={2} separator="," /> : value} | ||
| </Typography> | ||
|
|
||
| {/* Coordinator */} | ||
| {description && ( | ||
| <Typography variant="body2" sx={{ color: darkMode ? "#a9a9a9" : "#000", marginTop: "8px" }}> | ||
| <Typography variant="body2" sx={{ color: "#fff", marginTop: "8px" }}> | ||
| {description} | ||
| </Typography> | ||
| )} | ||
|
|
||
| {/* Additional Info (View Link Aligned to Right with Bright Blue Color and Arrow) */} | ||
| {additionalInfo && ( | ||
| <div style={{ marginTop: "10px", display: "flex", justifyContent: "flex-end", alignItems: "center" }}> | ||
| <a | ||
| href={`/events/${title.toLowerCase().replace(/\s+/g, "-")}`} | ||
| style={{ | ||
| textDecoration: "none", | ||
| color: "#007BFF", // Bright Blue Color | ||
| color: "#fff", | ||
| fontWeight: "bold", | ||
| display: "flex", | ||
| alignItems: "center", | ||
| }} | ||
| > | ||
| View | ||
| <ArrowForward sx={{ marginLeft: "5px", color: "#007BFF" }} /> {/* Bright Blue Arrow */} | ||
| <ArrowForward sx={{ marginLeft: "5px", color: "#fff" }} /> |
There was a problem hiding this comment.
All text colors have been changed to white (#fff), which will make the text invisible or hard to read when the bgColor is light or when no bgColor is provided (defaulting to light gray #f7f7f7). This removes the original dynamic color logic that adapted to dark/light modes.
| @@ -12,8 +14,5 @@ | |||
| "client": "cd frontend && npm start", | |||
| "update-client": "cd frontend && npm install", | |||
| "update-server": "cd backend && npm install" | |||
There was a problem hiding this comment.
The devDependencies section containing eslint has been removed. This will remove linting capabilities from the project, which is important for code quality maintenance. Unless ESLint configuration was moved elsewhere, this removal may degrade code quality standards.
| "update-server": "cd backend && npm install" | |
| "update-server": "cd backend && npm install" | |
| }, | |
| "devDependencies": { | |
| "eslint": "^8.0.0" |
| <VisualizationCard title="Revenue and Event Day by Day" chartType="line" height="22vh" /> | ||
| </Grid> | ||
|
|
||
| {/* Total Participation */} | ||
| <Grid item> | ||
| <VisualizationCard title="Total Participation" chartType="line" height="22vh" /> | ||
| <Grid item xs={12} sm={6} md={3}> | ||
| <VisualizationCard title="Revenue and Event Day by Day" chartType="line" height="22vh" /> | ||
| </Grid> | ||
| <Grid item xs={12} sm={6} md={3}> | ||
| <VisualizationCard title="Revenue and Event Day by Day" chartType="line" height="22vh" /> |
There was a problem hiding this comment.
Three visualization cards have identical titles "Revenue and Event Day by Day". This appears to be copy-paste code that wasn't updated with unique, meaningful titles for each card. Each visualization should have a distinct title reflecting its actual purpose.
| @@ -0,0 +1,16 @@ | |||
| const express = require('express'); | |||
| const router = express.Router(); | |||
| const mongoose = require('mongoose'); | |||
There was a problem hiding this comment.
Unused variable mongoose.
| const mongoose = require('mongoose'); |
| import Layout from "../layouts/layout"; // Import your Layout component | ||
| import React, { useState, useEffect, useContext } from "react"; | ||
| import { useNavigate } from "react-router-dom"; | ||
| import MetricCard from "../components/metricCard"; |
There was a problem hiding this comment.
Unused import MetricCard.
| import MetricCard from "../components/metricCard"; |
| import React, { useState, useEffect, useContext } from "react"; | ||
| import { useNavigate } from "react-router-dom"; | ||
| import MetricCard from "../components/metricCard"; | ||
| import VisualizationCard from "../components/visualizationCard"; |
There was a problem hiding this comment.
Unused import VisualizationCard.
| import VisualizationCard from "../components/visualizationCard"; |
| const navigate = useNavigate(); | ||
| const socket = useContext(WebSocketContext); // Access global WebSocket instance | ||
| const objID = Cookies.get("objId"); | ||
| const [socketError, setSocketError] = useState(null); // State to track errors |
There was a problem hiding this comment.
Unused variable socketError.
| event: "", | ||
| mobile: "", | ||
| status: "active", | ||
| if (socket && !hasJoinedRoom) { |
There was a problem hiding this comment.
This negation always evaluates to true.
No description provided.