-
Notifications
You must be signed in to change notification settings - Fork 0
Qa/cleanup #56
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.
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
Qa/cleanup #56
Conversation
There was a problem hiding this 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 pull request implements a series of minor bug fixes, code cleanups, and UX improvements across both front-end and back-end code. Key updates include:
- Updates to user validation and API error responses using the new validateUser function.
- Replacement of deprecated components (such as CircularProgress) with improved alternatives (DonutChart) and revised WebSocket handling in LivePoll.
- Code style, dependency, and import refactoring to support a cleaner and more consistent codebase.
Reviewed Changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| services/userCourse.ts | Made the role parameter optional in validateUser and adjusted usage accordingly. |
| models/Analytics.ts | Introduced new analytics types (note potential typo in type name). |
| lib/utils.ts & lib/constants.ts | Updated imports and type references to align with updated analytics models. |
| hooks/use-poll-socket.ts | Removed callback dependencies from the effect hook for WebSocket setup. |
| components/* | Refactored various UI components including DonutChart, Tooltip, LivePoll, and PastQuestions. |
| app/dashboard/* & app/api/* | Updated API endpoints with unified user validation and modified error handling. |
Comments suppressed due to low confidence (6)
services/userCourse.ts:51
- The validateUser function now accepts an optional role. Please ensure that all callers handle undefined role values appropriately and update the documentation to reflect the intended behavior.
role?: Role,
models/Analytics.ts:3
- [nitpick] The type name 'QuestionWithResponesAndOptions' appears to contain a typo. Consider renaming it to 'QuestionWithResponsesAndOptions' for clarity.
export type QuestionWithResponesAndOptions = ResponseWithOptions & Question;
app/dashboard/course/[courseId]/(lecturerPages)/layout.tsx:70
- [nitpick] Using encodeURIComponent on the tab name for routing may produce unexpected URLs. Please verify that the resulting URL matches your routing structure.
router.push(encodeURIComponent(tab.toLowerCase()));
app/api/updateCourse/[id]/route.ts:39
- The error status changed from 403 to 404 for unauthorized access. Confirm that this change aligns with the overall API design and client expectations.
if (!courseId || !(await validateUser(session.user.id, courseId, Role.LECTURER))) {
app/dashboard/course/[courseId]/start-session/page.tsx:410
- [nitpick] The left margin value has been reduced significantly. Please verify that the new margin meets the design requirements and does not adversely affect layout consistency.
margin={{ left: 10, right: 20, top: 20, bottom: 20 }}
app/dashboard/course/[courseId]/(lecturerPages)/admin/page.tsx:1
- [nitpick] The import path contains a spelling mistake ('AddInstuctorForm' instead of 'AddInstructorForm'). Consider renaming the file and the reference for consistency.
import { AddInstructorForm } from "@/components/AddInstuctorForm";
| } | ||
| }; | ||
| }, [courseSessionId, userId, onConnect, onDisconnect, onMessage]); | ||
| }, [courseSessionId, userId]); |
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The dependency array omits the onMessage callback (and possibly onConnect/onDisconnect). Consider including these callbacks to prevent potential stale closures if they are updated.
| }, [courseSessionId, userId]); | |
| }, [courseSessionId, userId, onMessage, onConnect, onDisconnect]); |
| className={`w-3 h-3 rounded-full ${wsRef.current?.OPEN ? "bg-green-500" : "bg-red-500"}`} | ||
| /> | ||
| <span>{isConnected ? "Connected" : "Disconnected"}</span> | ||
| <span>{wsRef.current?.OPEN ? "Connected" : "Disconnected"}</span> |
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebSocket instances do not have an 'OPEN' property. Consider checking the connection state using 'wsRef.current?.readyState === WebSocket.OPEN' instead.
Pull Request Template
Referenced Issue: N/A
Reviewers (if applicable): Add Here
Note
Inform your reviewers using Slack on PRs in addition to tagging them on the PR.
Description of changes
Set of fixes for minor bugs, minor code and UX improvements
Important
Run a local build first to ensure everything is working