Intro Course: Seat Plan Assignment
#279
Conversation
# Conflicts: # clients/intro_course_developer_component/src/introCourse/interfaces/PostDeveloperProfile.ts
WalkthroughThis pull request introduces a comprehensive seat assignment feature. It adds a new route for seat assignments, a sidebar menu item, UI components for managing seat assignments, and several custom hooks for handling assignments, filtering, file uploads, and CSV downloads. On the server side, SQL migrations, queries, and a new Go module for seat plan management have been implemented. These changes establish endpoints for creating, updating, retrieving, and deleting seat plans, along with related data models and utilities across both the client and server layers. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SPA as SeatAssignmentPage
participant HS as Custom Hooks
participant API as SeatPlan API
participant DB as Database
U->>SPA: Open seat assignment page
SPA->>HS: Request seat plan data (useQuery / getSeatPlan)
HS->>API: GET /seat_plan
API->>DB: Query seat records
DB-->>API: Return seat data
API-->>HS: Send seat plan data
HS-->>SPA: Display seat assignments
U->>SPA: Modify assignments (update, upload CSV, etc.)
SPA->>HS: Trigger updateSeatPlan() / createSeatPlan() / deleteSeatPlan()
HS->>API: PUT/POST/DELETE /seat_plan
API->>DB: Update corresponding seat records
DB-->>API: Confirmation of update
API-->>HS: Return success message
HS-->>SPA: Update UI accordingly
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (59)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/interfaces/DeveloperWithProfile.ts (1)
1-7: Well-defined composite type for developer data.The
DeveloperWithProfiletype correctly combines course participation data with an optional developer profile, allowing for cases where a profile might not exist. The type is properly exported and structured.Consider reviewing the relative import path (
../../../interfaces/DeveloperProfile). Deep nesting in import paths can sometimes indicate an opportunity to restructure the project organization or use path aliases.clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/utils/rechnerHalle.ts (1)
1-29: Add documentation and TypeScript typing to improve clarityThe constant
RECHNERHALLE_SEATSprovides what appears to be a predefined set of seat identifiers for the "Rechnerhalle" layout, but lacks documentation explaining the meaning of the identifiers' format (e.g., what does '1-1-1' represent?). Adding documentation would help other developers understand the purpose and pattern of these seat identifiers.Additionally, consider adding proper TypeScript typing to make the code more maintainable:
+/** + * Predefined seat identifiers for the Rechnerhalle layout. + * Format: '{floor}-{row}-{position}' + */ -export const RECHNERHALLE_SEATS = [ +export const RECHNERHALLE_SEATS: string[] = [ '1-1-1', // ... ]servers/intro_course/seatPlan/validation.go (1)
3-12: Consider exporting the validation function and adding documentationThe
validateSeatNamesfunction is not exported (doesn't start with an uppercase letter), which means it can only be used within theseatPlanpackage. If this function needs to be used by other packages, consider exporting it by renaming it toValidateSeatNames.Additionally, adding documentation comments would improve code maintainability:
+// ValidateSeatNames checks if all seat names in the provided slice are unique. +// Returns true if all names are unique, false if any duplicates are found. -func validateSeatNames(seatNames []string) bool { +func ValidateSeatNames(seatNames []string) bool { // existing implementation }The current implementation efficiently checks for duplicates using a map with O(n) time complexity, which is appropriate for this use case.
clients/intro_course_developer_component/src/introCourse/network/mutations/createCustomKeycloakGroup.ts (1)
2-2: Import path changed from absolute to relativeThe import path for
CreateKeycloakGrouphas been changed from an absolute path to a relative path. This is fine, but ensure this change maintains consistency with the project's import style convention.If the project generally uses absolute imports (starting with
src/or@/), consider maintaining that convention. If moving toward relative imports, ensure this change is part of a broader consistent approach.clients/intro_course_developer_component/src/introCourse/network/queries/getSeatPlan.ts (1)
4-13: Improve error handling and consider request optimizationThe function correctly fetches seat plans and has basic error handling, but consider these improvements:
- Enhance error handling with more specific error messages instead of just logging and rethrowing:
try { return ( await introCourseAxiosInstance.get(`intro-course/api/course_phase/${coursePhaseID}/seat_plan`) ).data } catch (err) { console.error(err) - throw err + throw new Error(`Failed to fetch seat plan for course phase ${coursePhaseID}: ${err.message}`) }
- Consider adding a timeout to the request to prevent hanging if the server is unresponsive:
return ( - await introCourseAxiosInstance.get(`intro-course/api/course_phase/${coursePhaseID}/seat_plan`) + await introCourseAxiosInstance.get(`intro-course/api/course_phase/${coursePhaseID}/seat_plan`, { + timeout: 10000 // 10 seconds timeout + }) ).data
- For production code, consider using a more structured logging approach than
console.error.clients/intro_course_developer_component/src/introCourse/network/mutations/deleteSeatPlan.ts (1)
3-17: Function implementation looks clear and follows standard practice for API calls.The function correctly uses async/await pattern with proper error handling. The Content-Type header
application/json-path+jsonis unusual though - typically DELETE requests don't have request bodies that would need a Content-Type.Consider removing the Content-Type header as it appears unnecessary for DELETE requests since no request body is being sent.
export const deleteSeatPlan = async (coursePhaseID: string): Promise<void> => { try { await introCourseAxiosInstance.delete( `intro-course/api/course_phase/${coursePhaseID}/seat_plan`, - { - headers: { - 'Content-Type': 'application/json-path+json', - }, - }, ) } catch (err) { console.error(err) throw err } }clients/intro_course_developer_component/src/introCourse/network/mutations/createSeatPlan.ts (1)
3-18: Function implementation looks good but has a potential Content-Type mismatch.The function uses proper error handling and correctly accepts parameters. However, the Content-Type header
application/json-path+jsonseems inconsistent with sending an array directly as the request body.The Content-Type header
application/json-path+jsondoesn't match the actual data being sent (a regular JavaScript array). Consider either:
- Using standard
application/jsoncontent type:export const createSeatPlan = async (coursePhaseID: string, seatNames: string[]): Promise<void> => { try { await introCourseAxiosInstance.post( `intro-course/api/course_phase/${coursePhaseID}/seat_plan`, seatNames, { headers: { - 'Content-Type': 'application/json-path+json', + 'Content-Type': 'application/json', }, }, ) } catch (err) { console.error(err) throw err } }
- Or if the API specifically expects the custom content type, ensure the data format matches what the server expects.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/utils/fileUpload.ts (2)
8-13: CSV parsing logic could be more robust.The current implementation splits by both newlines and commas using a single regex, which might not handle common CSV edge cases correctly.
Consider enhancing the CSV parsing to handle more complex cases like quoted values containing commas or newlines:
export const parseCSV = (content: string): string[] => { - return content - .split(/[\r\n,]+/) - .map((seat) => seat.trim()) - .filter((seat) => seat.length > 0) + // First split by lines, then by commas on each line + return content + .split(/\r?\n/) + .flatMap(line => line.split(',')) + .map(seat => seat.trim()) + .filter(seat => seat.length > 0) }For a more robust solution, consider using a dedicated CSV parsing library if this feature needs to handle complex CSV files in the future.
21-41: File reading implementation is well structured but could have stronger typechecking.The implementation correctly uses FileReader with proper error handling, but has a potential type issue with
e.target?.result.Add a safeguard against null or undefined
e.targetand explicitly check if the result is a string:reader.onload = (e) => { try { - const content = e.target?.result as string + if (!e.target || typeof e.target.result !== 'string') { + throw new Error('Failed to read file content') + } + const content = e.target.result const seatNames = parseCSV(content) resolve(seatNames) } catch (error) { reject(new Error('Failed to parse CSV file. Please check the format.')) } }clients/intro_course_developer_component/src/introCourse/interfaces/Seat.ts (1)
1-7: Type definition looks good, consider adding JSDoc comments.The
Seattype is well-defined with appropriate property types. For better maintainability and developer experience, consider adding JSDoc comments to document the purpose of each property.+/** + * Represents a seat in the classroom with its properties and assignments + */ export type Seat = { + /** The unique identifier/name of the seat */ seatName: string + /** Indicates whether the seat has a Mac computer */ hasMac: boolean + /** The identifier of the device at this seat, if any */ deviceID: string | null + /** The identifier of the student assigned to this seat, if any */ assignedStudent: string | null + /** The identifier of the tutor assigned to this seat, if any */ assignedTutor: string | null }clients/intro_course_developer_component/src/introCourse/network/mutations/updateSeatPlan.ts (1)
15-18: Consider enhancing error logging.The current error handling logs the error object but doesn't provide context about what operation failed.
} catch (err) { - console.error(err) + console.error(`Failed to update seat plan for course phase ${coursePhaseID}:`, err) throw err }servers/intro_course/seatPlan/main.go (1)
10-16: Add documentation and consider error handlingThis initialization function looks good, but would benefit from a few improvements:
- Add a function comment explaining the purpose of this seat plan module
- Consider adding error handling or a return value to signal successful initialization
- Consider adding a check to prevent multiple initializations of the singleton
+// InitSeatPlanModule sets up the seat planning functionality for the intro course. +// It initializes routes with authentication and creates a seat plan service singleton. func InitSeatPlanModule(routerGroup *gin.RouterGroup, queries db.Queries, conn *pgxpool.Pool) { + // Don't reinitialize if already set up + if SeatPlanServiceSingleton != nil { + return + } setupSeatPlanRouter(routerGroup, keycloakTokenVerifier.AuthenticationMiddleware) SeatPlanServiceSingleton = &SeatPlanService{ queries: queries, conn: conn, } }clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/ResetSeatAssignmentDialog.tsx (3)
14-22: Consider making the dialog controllable from parent and add loading stateThe component interface could be improved to allow more control from the parent component:
interface ResetSeatAssignmentDialogProps { disabled: boolean - onSuccess: () => void + onSuccess: () => Promise<void> | void + isOpen?: boolean + onOpenChange?: (open: boolean) => void + isLoading?: boolean }
23-35: Add loading state and improve user feedback during reset operationThe component doesn't provide feedback during the reset operation. Consider adding loading state and handling async operations:
export const ResetSeatAssignmentDialog = ({ disabled, onSuccess, + isOpen, + onOpenChange, + isLoading = false, }: ResetSeatAssignmentDialogProps): JSX.Element => { - const [isDialogOpen, setIsDialogOpen] = useState(false) + const [isDialogOpen, setIsDialogOpen] = useState(isOpen || false) + + // Sync internal state with props + useEffect(() => { + if (isOpen !== undefined) { + setIsDialogOpen(isOpen); + } + }, [isOpen]); + + const handleOpenChange = (open: boolean) => { + setIsDialogOpen(open); + onOpenChange?.(open); + } return ( - <Dialog open={isDialogOpen} onOpenChange={setIsDialogOpen}> + <Dialog open={isDialogOpen} onOpenChange={handleOpenChange}> <DialogTrigger asChild> <Button variant='outline' className='text-red-600 border-red-200 hover:bg-red-50' disabled={disabled} > - <RefreshCw className='mr-2 h-4 w-4' /> + <Trash className='mr-2 h-4 w-4' /> Reset Assignments </Button>Note: You would need to import the
Trashicon from lucide-react and add theuseEffectimport.
48-56: Handle async operations and provide feedback during resetThe current implementation assumes the reset operation completes immediately. Consider adding loading state and handling async operations:
<Button variant='destructive' + disabled={isLoading} onClick={() => { - onSuccess() - setIsDialogOpen(false) + Promise.resolve(onSuccess()).then(() => { + setIsDialogOpen(false); + if (onOpenChange) { + onOpenChange(false); + } + }); }} > - Yes, Reset Assignments + {isLoading ? ( + <> + <Loader2 className="mr-2 h-4 w-4 animate-spin" /> + Resetting... + </> + ) : ( + "Yes, Reset Assignments" + )} </Button>Note: You would need to import the
Loader2icon from lucide-react.servers/intro_course/seatPlan/seatPlanDTO/seat.go (3)
8-14: Clarify comments about pgtype usageThe comment about using pgtype for empty values only appears on the
AssignedStudentfield, but it applies to all pgtype fields. Consider moving this comment to the struct level or applying it to all relevant fields:type Seat struct { SeatName string `json:"seatName"` HasMac bool `json:"hasMac"` - DeviceID pgtype.Text `json:"deviceID"` - AssignedStudent pgtype.UUID `json:"assignedStudent"` // using pgtype bc. it might be empty - AssignedTutor pgtype.UUID `json:"assignedTutor"` + DeviceID pgtype.Text `json:"deviceID"` // using pgtype because it might be empty + AssignedStudent pgtype.UUID `json:"assignedStudent"` // using pgtype because it might be empty + AssignedTutor pgtype.UUID `json:"assignedTutor"` // using pgtype because it might be empty }
16-24: Consider adding a conversion function from DTO to DB modelThe code provides functions to convert from DB models to DTOs, but not vice versa. For completeness, consider adding a function to convert from DTO to DB model, which might be useful for update operations:
func GetSeatDTOFromDBModel(seat db.Seat) Seat { return Seat{ SeatName: seat.SeatName, HasMac: seat.HasMac, DeviceID: seat.DeviceID, AssignedStudent: seat.AssignedStudent, AssignedTutor: seat.AssignedTutor, } } + +// GetDBModelFromSeatDTO converts a Seat DTO to a db.Seat model +func GetDBModelFromSeatDTO(seat Seat, coursePhaseID pgtype.UUID) db.Seat { + return db.Seat{ + CoursePhaseID: coursePhaseID, + SeatName: seat.SeatName, + HasMac: seat.HasMac, + DeviceID: seat.DeviceID, + AssignedStudent: seat.AssignedStudent, + AssignedTutor: seat.AssignedTutor, + } +}
26-32: Consider adding validation in the conversion functionsThe current implementation doesn't include any validation. Consider adding basic validation to ensure the integrity of the data:
func GetSeatDTOsFromDBModels(seats []db.Seat) []Seat { + if seats == nil { + return []Seat{} + } seatDTOs := make([]Seat, 0, len(seats)) for _, seat := range seats { seatDTOs = append(seatDTOs, GetSeatDTOFromDBModel(seat)) } return seatDTOs }clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatPlanDialog.tsx (2)
40-42: Improve key and null handling for seat itemsThe current implementation uses
seat.seatName ?? indexas a React key, but then directly referencesseat.seatNamefor display without a null check. This inconsistency could lead to rendering "null" or "undefined" text ifseatNameis nullish.- <li key={seat.seatName ?? index}>{seat.seatName}</li> + <li key={seat.seatName ?? index}>{seat.seatName || 'Unnamed Seat'}</li>
30-30: Consider virtualization for large seat plansThe dialog uses
max-h-[80vh]withoverflow-y-autowhich works well for small to medium sized lists, but could cause performance issues with very large seat plans. Consider implementing virtualization for better performance with large datasets.clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useUpdateSeats.ts (2)
6-20: Enhance error handling with specific error messagesThe hook uses a generic error message that doesn't provide specific information about what went wrong. Consider using the actual error message from the API response for more actionable feedback.
- onError: () => { - setError('Failed to update. Please try again.') - }, + onError: (error: Error) => { + setError(`Failed to update: ${error.message || 'Please try again.'}`) + },
11-11: Add validation for phaseIdThe current implementation falls back to an empty string if
phaseIdis null or undefined, but there's no validation to ensure it's a valid ID before making the API call.- mutationFn: (updatedSeats: Seat[]) => updateSeatPlan(phaseId ?? '', updatedSeats), + mutationFn: (updatedSeats: Seat[]) => { + if (!phaseId) throw new Error('Phase ID is required'); + return updateSeatPlan(phaseId, updatedSeats); + },clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx (1)
6-30: Consider adding configurable filter logic (AND/OR)Currently, all filter conditions must be met (using AND logic in the return statement) for a seat to be included in the results. This might be too restrictive in some cases. Consider making the filter logic configurable.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/TutorAssignmentFilter.tsx (2)
10-23: Consider importing TutorAssignmentFilterOptions instead of redefining interfaceThe interface for filter options is defined inline, but based on the imports in other files, there seems to be a
TutorAssignmentFilterOptionsinterface already defined. Consider importing and reusing that interface to avoid duplication.- interface TutorAssignmentFilterProps { - filterOptions: { - showAssigned: boolean - showUnassigned: boolean - showWithMac: boolean - showWithoutMac: boolean - } - setFilterOptions: (filterOptions: { - showAssigned: boolean - showUnassigned: boolean - showWithMac: boolean - showWithoutMac: boolean - }) => void - } + import { TutorAssignmentFilterOptions } from '../interfaces/TutorAssignmentFilterOptions' + + interface TutorAssignmentFilterProps { + filterOptions: TutorAssignmentFilterOptions + setFilterOptions: (filterOptions: TutorAssignmentFilterOptions) => void + }
32-35: Add aria-label for better accessibilityThe button shows only an icon on small screens which could be an accessibility issue. Add an aria-label to ensure screen readers can properly describe the button's function.
- <Button variant='outline' className='gap-1'> + <Button variant='outline' className='gap-1' aria-label="Filter seats">servers/intro_course/db/query/seat.sql (3)
1-4: SQL query looks good, but could benefit from a comment describing the array parameterThe CreateSeatPlan implementation using unnest() to convert an array into rows is efficient. This is a good approach for bulk insertion of seat records.
Consider adding a comment describing the expected format of the "seats" array parameter for better documentation:
-- name: CreateSeatPlan :exec + -- seats: Array of seat names to be created for the given course phase INSERT INTO seat (course_phase_id, seat_name) SELECT $1, s FROM unnest(sqlc.arg(seats)::text[]) AS s;
6-10: Consider using specific column names instead of SELECT * for better maintainabilityWhile SELECT * works for now, explicitly listing columns provides better maintainability if the table schema changes in the future.
-- name: GetSeatPlan :many -SELECT * +SELECT course_phase_id, seat_name, has_mac, device_id, assigned_student, assigned_tutor FROM seat WHERE course_phase_id = $1 ORDER BY seat_name;
21-23: Consider adding a safety comment for DeleteSeatPlanThis query deletes all seats for a course phase without any additional confirmation in the SQL itself, which is expected but worth noting.
-- name: DeleteSeatPlan :exec +-- Warning: Deletes ALL seats for the specified course phase DELETE FROM seat WHERE course_phase_id = $1;clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/SeatAssignmentPage.tsx (3)
1-19: Import statements look good, but consider grouping themThe imports are comprehensive for the component's needs, but could be better organized.
Consider grouping imports by type/source for better maintainability:
- External libraries (React, react-router, etc.)
- Shared UI components
- Internal interfaces/types
- Internal queries/hooks
- Local components
20-62: Consider using react-query's useQueries hook for multiple queriesThe component has four separate useQuery calls that all depend on the same phaseId. This could lead to waterfall fetching.
Consider using the useQueries hook to batch these requests:
const results = useQueries({ queries: [ { queryKey: ['tutors', phaseId], queryFn: () => getAllTutors(phaseId ?? ''), }, { queryKey: ['participants', phaseId], queryFn: () => getCoursePhaseParticipations(phaseId ?? ''), }, // other queries... ], }); const [tutorsResult, participationsResult, profilesResult, seatsResult] = results;This would allow parallel data fetching rather than potentially sequential fetching.
98-119: Consider using a guard clause pattern for conditional renderingThe component repeats the
seats.length > 0check multiple times, which could be simplified.Consider using a guard clause pattern or extracting a component for better readability:
// Within the return statement if (seats.length === 0) { return ( <div className='space-y-6'> <ManagementPageHeader>Seat Assignment</ManagementPageHeader> <SeatUploader existingSeats={seats || []} /> </div> ); } // Or continue with all components return ( <div className='space-y-6'> <ManagementPageHeader>Seat Assignment</ManagementPageHeader> <SeatUploader existingSeats={seats || []} /> <SeatMacAssigner existingSeats={seats} /> <SeatTutorAssigner seats={seats} tutors={tutors || []} numberOfStudents={developerWithProfiles.length} /> <SeatStudentAssigner seats={seats} developerWithProfiles={developerWithProfiles} tutors={tutors} /> </div> );clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx (1)
38-47: Add error handling for the file download processThe current implementation doesn't handle potential errors during the Blob creation or download process.
Add try/catch block to handle potential errors:
- const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }) - const url = URL.createObjectURL(blob) - const link = document.createElement('a') - link.setAttribute('href', url) - link.setAttribute('download', 'seat_assignments.csv') - link.style.visibility = 'hidden' - document.body.appendChild(link) - link.click() - document.body.removeChild(link) + try { + const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }) + const url = URL.createObjectURL(blob) + const link = document.createElement('a') + link.setAttribute('href', url) + link.setAttribute('download', 'seat_assignments.csv') + link.style.visibility = 'hidden' + document.body.appendChild(link) + link.click() + document.body.removeChild(link) + // Release the object URL to free memory + URL.revokeObjectURL(url) + } catch (error) { + console.error('Error downloading CSV file:', error) + // Consider adding a user-friendly error notification here + }clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatUploader.tsx (4)
28-33: State management looks good but consider including a success message.Your state management approach is solid, but it might enhance user experience to add a success state/message when operations complete successfully, similar to how you're handling errors.
86-104: Consider more specific error handling for file operations.The catch block currently handles all errors the same way. Consider differentiating between specific error types (like file format errors vs. network errors) for more user-friendly error messages.
try { const seatNames = await readCSVFile(file) // ... } catch (err: any) { - setError(err.message || 'An error occurred while uploading the file.') + if (err instanceof TypeError || err.message.includes('format')) { + setError('Invalid file format. Please ensure your CSV is properly formatted.') + } else if (err.message.includes('network') || err.code === 'NETWORK_ERROR') { + setError('Network error occurred. Please check your connection and try again.') + } else { + setError(err.message || 'An error occurred while uploading the file.') + } setIsUploading(false) }
128-133: Consider adding descriptive aria-label to Alert component.For better accessibility, add an aria-label to the Alert component to provide more context to screen reader users.
-<Alert variant='destructive' className='mb-4'> +<Alert variant='destructive' className='mb-4' aria-label="Error notification"> <AlertCircle className='h-4 w-4' /> <AlertDescription>{error}</AlertDescription> </Alert>
74-77: Consider adding a loading state indicator for the Rechnerhalle selection.The function correctly sets
isUploadingbut it could be more explicit about handling loading state.clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useAssignStudents.tsx (2)
15-15: Consider using a more robust shuffling algorithm.The current shuffling algorithm (
array.slice().sort(() => Math.random() - 0.5)) doesn't provide uniform randomization. For more statistically sound results, consider implementing Fisher-Yates.-const shuffleArray = <T,>(array: T[]): T[] => array.slice().sort(() => Math.random() - 0.5) +const shuffleArray = <T,>(array: T[]): T[] => { + const result = array.slice(); + for (let i = result.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + [result[i], result[j]] = [result[j], result[i]]; + } + return result; +}
44-56: Consider extracting helper functions for better readability.The seat and student categorization logic could be extracted into helper functions to make the code more maintainable.
+// Helper functions to categorize students and seats +const categorizeStudentsByMac = (devs: DeveloperWithProfile[]) => { + return { + withMacs: devs.filter((dev) => dev.profile?.hasMacBook === true), + withoutMacs: devs.filter((dev) => dev.profile?.hasMacBook === false), + unknownMac: devs.filter((dev) => dev.profile?.hasMacBook === undefined), + } +} + +const categorizeSeats = (seats: Seat[]) => { + const eligibleSeats = seats.filter((seat) => seat.assignedTutor) + return { + withMacs: eligibleSeats.filter((seat) => seat.hasMac), + withoutMacs: eligibleSeats.filter((seat) => !seat.hasMac), + } +} // In the useCallback: // Create a copy and clear any existing student assignments const updatedSeats: Seat[] = seats.map((seat) => ({ ...seat, assignedStudent: null })) -const eligibleSeats = updatedSeats.filter((seat) => seat.assignedTutor) - -// Separate students by Mac ownership -const studentsWithMacs = developerWithProfiles.filter((dev) => dev.profile?.hasMacBook === true) -const studentsWithoutMacs = developerWithProfiles.filter( - (dev) => dev.profile?.hasMacBook === false, -) -const studentsUnknownMac = developerWithProfiles.filter( - (dev) => dev.profile?.hasMacBook === undefined, -) - -// Separate eligible seats by Mac availability -const seatsWithMacs = eligibleSeats.filter((seat) => seat.hasMac) -const seatsWithoutMacs = eligibleSeats.filter((seat) => !seat.hasMac) + +// Use helper functions for categorization +const { withMacs: studentsWithMacs, withoutMacs: studentsWithoutMacs, unknownMac: studentsUnknownMac } = + categorizeStudentsByMac(developerWithProfiles) +const { withMacs: seatsWithMacs, withoutMacs: seatsWithoutMacs } = + categorizeSeats(updatedSeats)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/SeatTutorTable.tsx (3)
67-102: Refactor shift-selection logic for clarity.The shift-click selection logic is complex and could be made more maintainable by extracting it into a separate helper function.
+// Helper function to handle shift selection +const handleShiftSelection = ( + lastIndex: number, + currentIndex: number, + allItems: Seat[], + currentSelection: string[], + newState: boolean +): string[] => { + const startIndex = Math.min(lastIndex, currentIndex); + const endIndex = Math.max(lastIndex, currentIndex); + const itemsInRange = allItems + .slice(startIndex, endIndex + 1) + .map((item) => item.seatName); + + if (newState) { + // Add all items in the range to the current selection + return Array.from(new Set([...currentSelection, ...itemsInRange])); + } else { + // Remove all items in the range from the current selection + return currentSelection.filter((name) => !itemsInRange.includes(name)); + } +}; const handleCheckboxClick = ( e: React.MouseEvent<HTMLButtonElement, MouseEvent>, seatName: string, currentIndex: number, ) => { const isCurrentlySelected = selectedSeatNames.includes(seatName) // Determine new selection state for the clicked checkbox const newSelectedState = !isCurrentlySelected if (e.shiftKey && lastSelectedIndex !== null) { - // Calculate the range in the filtered seats list based on indices - const startIndex = Math.min(lastSelectedIndex, currentIndex) - const endIndex = Math.max(lastSelectedIndex, currentIndex) - const seatsInRange = filteredSeats - .slice(startIndex, endIndex + 1) - .map((seat) => seat.seatName) - - let newSelection: string[] - if (newSelectedState) { - // Add all seats in the range to the current selection - newSelection = Array.from(new Set([...selectedSeatNames, ...seatsInRange])) - } else { - // Remove all seats in the range from the current selection - newSelection = selectedSeatNames.filter((name) => !seatsInRange.includes(name)) - } - setSelectedSeatNames(newSelection) + const newSelection = handleShiftSelection( + lastSelectedIndex, + currentIndex, + filteredSeats, + selectedSeatNames, + newSelectedState + ); + setSelectedSeatNames(newSelection); } else { // Normal toggle if shift key is not pressed setSelectedSeatNames((prev) => prev.includes(seatName) ? prev.filter((name) => name !== seatName) : [...prev, seatName], ) } // Update last selected index for future shift selections setLastSelectedIndex(currentIndex) }
137-140: Use type-safe approach for indeterminate checkbox state.The current checkbox state handling could be improved for type safety.
<Checkbox - checked={areAllSelected || (areSomeSelected && 'indeterminate')} + checked={areAllSelected} + {...(areSomeSelected && !areAllSelected ? { checked: 'indeterminate' as const } : {})} onCheckedChange={handleSelectAll} aria-label='Select all seats' />
148-197: Consider using virtualization for large seat lists.For potentially large lists of seats, consider implementing virtualization (e.g., with react-window or react-virtualized) to improve performance.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx (3)
63-69: Consider resetting all seats for consistency.The current implementation only resets seats that have an assigned student. Consider resetting all seats for consistency, even if the result is the same.
const resetAssignments = useCallback(() => { - const updatedSeats = seats - .filter((seat) => seat.assignedStudent != null) - .map((seat) => ({ ...seat, assignedStudent: null })) + const updatedSeats = seats.map((seat) => + seat.assignedStudent != null ? { ...seat, assignedStudent: null } : seat + ) mutation.mutate(updatedSeats) setAssignmentStatus('none') }, [seats, mutation])
159-166: Optimize student lookup with a Map for better performance.Using
findinside a loop can be inefficient for large datasets. Consider using a Map for faster lookups.<TableBody> + {(() => { + // Create a lookup map outside the loop for better performance + const studentMap = new Map( + developerWithProfiles.map(dev => [dev.participation.student.id, dev]) + ); + + return seats .filter((seat) => seat.assignedStudent) .sort((a, b) => a.seatName.localeCompare(b.seatName)) .map((seat) => { - const student = developerWithProfiles.find( - (dev) => dev.participation.student.id === seat.assignedStudent, - ) + const student = studentMap.get(seat.assignedStudent || ''); return ( <TableRow key={seat.seatName}> {/* ... rest of the row rendering ... */} </TableRow> ) }) + })()} </TableBody>
138-144: Improve button disabled state UX with tooltip.The Assign Students button is disabled when students are already assigned, but doesn't explain why. Consider adding a tooltip.
-<Button - onClick={assignStudents} - disabled={mutation.isPending || assignedStudents > 0} -> - <UserCheck className='mr-2 h-4 w-4' /> - {mutation.isPending ? 'Assigning...' : 'Assign Students'} -</Button> +<div className="relative inline-block"> + <Button + onClick={assignStudents} + disabled={mutation.isPending || assignedStudents > 0} + aria-label={assignedStudents > 0 ? "Reset assignments before assigning again" : "Assign students to seats"} + > + <UserCheck className='mr-2 h-4 w-4' /> + {mutation.isPending ? 'Assigning...' : 'Assign Students'} + </Button> + {assignedStudents > 0 && ( + <div className="absolute left-1/2 -translate-x-1/2 bottom-full mb-2 px-3 py-1 bg-gray-800 text-white text-xs rounded whitespace-nowrap opacity-0 group-hover:opacity-100 transition-opacity"> + Reset assignments first + </div> + )} +</div>servers/intro_course/seatPlan/router.go (5)
14-26: Centralize coursePhaseID parsing or unify repeated patterns
Repetitive parsing ofcoursePhaseIDin each endpoint could lead to duplication. Consider extracting this logic into a small middleware or helper function to keep routes more concise and maintainable.
28-56: CreateSeatPlan: Validate seatNames more comprehensively
You’re only checking for uniqueness. Consider also validating name format or length constraints if relevant. Ensuring seat names adhere to certain patterns or lengths can prevent future data inconsistencies.
58-73: getSeatPlan: Provide more granular HTTP status codes
When the seat plan doesn’t exist or the coursePhaseID is invalid, returning a 404 (Not Found) or 400 (Bad Request) might be more descriptive than a 500 (Internal Server Error).
98-113: deleteSeatPlan: Offer a soft-delete approach
Hard-deleting records can risk data loss. Depending on the use case, you might consider a soft-delete strategy (e.g., marking records inactive). This can simplify recovery if a seat plan is removed by mistake.
115-117: handleError: Follow a consistent JSON structure
Using"error"as a key is fine, but consider a more structured way of returning messages, e.g.,{ "message": "...", "details": { ... } }. This can facilitate standardized error handling on the client.clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/SeatTutorAssigner.tsx (4)
28-37: Consider a descriptive default collapse state
Currently,isCollapsedis set by checking if any seat already has a tutor assigned. This might be confusing for a new user if the component is hidden and they haven’t assigned anything yet.
39-45: handleTutorAssignment: Guard against stale seats data
Since you’re updating seat assignment by directly mutating the array reference, watch out for potential concurrency issues if multiple updates occur in quick succession. Using an immutable approach for seat updates (e.g., mapping to a new array) can help prevent subtle bugs with React state.- updatedSeat.assignedTutor = tutorId - mutation.mutate([updatedSeat]) + const updatedSeatCopy = { ...updatedSeat, assignedTutor: tutorId }; + mutation.mutate([updatedSeatCopy]);
63-74: Tutor distribution: Consider a descending sort
You sort in ascending order of assignment counts. Usually, one wants to see who’s assigned the most seats at the top. If you intend to highlight under-assigned tutors, ascending is fine. Just ensure it’s intentional.
163-172: Pass appropriate memoized or stable references to child components
When passing props likehandleTutorAssignmentandselectedSeatNames, always ensure they are stable references or memoized if necessary. This helps avoid unnecessary re-renders ofSeatTutorTable.clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatMacAssigner.tsx (2)
36-37: Initial collapse logic
Collapsing the card if any seat has a Mac can hide the assignment UI for all seats, even if some seats have no Mac assigned. Consider a separate user-driven toggle or a more nuanced default to avoid confusion.
45-54: handleToggleMac: Clear device ID carefully
You’re clearingdeviceIDwheneverhasMacis set to false. Just ensure that ifdeviceIDwas previously set, the user truly wants to lose it. Possibly prompt a confirmation or an “undo” approach.servers/intro_course/seatPlan/service.go (2)
22-38: CreateSeatPlan: Additional seat validation
You check for uniqueness inrouter.go. In the service layer, you might also confirm that seat names are non-empty or meet length requirements. This double-check can prevent invalid data from slipping in if the upstream validation is bypassed.
93-106: DeleteSeatPlan: Potential advanced usage
If seat data is needed for auditing, a “soft deletion” approach or archiving might be helpful. Permanent removal can hamper analytics or restore functionality.servers/intro_course/db/sqlc/seat.sql.go (2)
31-39: Consider adding transactional support for seat plan deletionThe current implementation properly handles the deletion of all seats for a specific course phase. For production scenarios, consider wrapping this operation in a transaction, especially if there might be referential integrity constraints or if seat deletion should be atomic with other operations.
75-104: Consider adding partial update functionalityThe current
UpdateSeatfunction requires all fields to be updated at once. In some scenarios, you might want to update only specific fields. Consider adding functions for partial updates to avoid unnecessary database writes.Also, the function doesn't check if the update actually affected any rows. You might want to return an error if no rows were updated to indicate that the requested seat wasn't found.
func (q *Queries) UpdateSeat(ctx context.Context, arg UpdateSeatParams) error { - _, err := q.db.Exec(ctx, updateSeat, + commandTag, err := q.db.Exec(ctx, updateSeat, arg.CoursePhaseID, arg.SeatName, arg.HasMac, arg.DeviceID, arg.AssignedStudent, arg.AssignedTutor, ) + if err != nil { + return err + } + if commandTag.RowsAffected() == 0 { + return fmt.Errorf("seat not found") + } return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seatPlan.csvis excluded by!**/*.csv
📒 Files selected for processing (37)
clients/intro_course_developer_component/routes/index.tsx(2 hunks)clients/intro_course_developer_component/sidebar/index.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/interfaces/Seat.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/network/mutations/createCustomKeycloakGroup.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/network/mutations/createSeatPlan.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/network/mutations/deleteSeatPlan.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/network/mutations/updateSeatPlan.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/network/queries/getSeatPlan.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/SeatAssignmentPage.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatMacAssigner.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/ResetSeatAssignmentDialog.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/SeatTutorAssigner.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/SeatTutorTable.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/TutorAssignmentFilter.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/ResetSeatPlanDialog.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatPlanDialog.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatUploader.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useAssignStudents.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useUpdateSeats.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/interfaces/DeveloperWithProfile.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/interfaces/TutorAssignmentFilterOptions.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/utils/fileUpload.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/utils/getTutorName.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/utils/rechnerHalle.ts(1 hunks)servers/intro_course/db/migration/0002_seat_plan.up.sql(1 hunks)servers/intro_course/db/query/seat.sql(1 hunks)servers/intro_course/db/sqlc/models.go(1 hunks)servers/intro_course/db/sqlc/seat.sql.go(1 hunks)servers/intro_course/main.go(2 hunks)servers/intro_course/seatPlan/main.go(1 hunks)servers/intro_course/seatPlan/router.go(1 hunks)servers/intro_course/seatPlan/seatPlanDTO/seat.go(1 hunks)servers/intro_course/seatPlan/service.go(1 hunks)servers/intro_course/seatPlan/validation.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
🔇 Additional comments (32)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/interfaces/TutorAssignmentFilterOptions.ts (1)
1-6: Well-structured filter options type definition.The
TutorAssignmentFilterOptionstype is cleanly defined with clear, descriptive property names that effectively support filtering seats based on assignment status and Mac availability.clients/intro_course_developer_component/sidebar/index.tsx (1)
21-25: Correctly implemented sidebar navigation item.The new "Seat Assignments" menu item follows the established pattern of the sidebar configuration, with proper permission requirements matching other administrative functions.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/utils/getTutorName.ts (1)
1-6: Clean utility function with proper error handling.The
getTutorNamefunction effectively retrieves a tutor's full name from an array of tutors, with appropriate fallback to 'Unknown Tutor' when no match is found. The implementation is efficient and handles null IDs correctly.clients/intro_course_developer_component/routes/index.tsx (2)
8-8: Import added correctly for the new Seat Assignment page.The new import for SeatAssignmentPage is properly structured.
30-34: New route implementation follows existing patterns.The new route for seat assignments maintains consistency with other routes in terms of structure and permission requirements.
servers/intro_course/main.go (1)
15-15: LGTM: Seat plan module integration follows established pattern.The integration of the seat plan module follows the established pattern in the codebase, using the same approach as other modules like developer profile and tutor modules.
Also applies to: 83-83
servers/intro_course/db/sqlc/models.go (2)
27-29: Ensure consistent null handling between Go and TypeScript.Note that the Go model uses
pgtype.Textandpgtype.UUIDfor nullable fields, while the TypeScript interface usesstring | null. Make sure the serialization/deserialization logic correctly handles these types consistently across the stack.
23-30:✅ Verification successful
Beware of modifying generated code.
The file contains a comment at the top indicating it's generated by sqlc and should not be edited manually. Adding the
Seatstruct directly to this file might lead to your changes being overwritten when the models are regenerated.Consider checking if you need to update the schema definition and regenerate the models instead of manually modifying this file:
🏁 Script executed:
#!/bin/bash # Check if there are SQL schema files that should define the Seat table fd -e sql "seat|Seat" --type file # Check if there are sqlc configuration files fd -g "sqlc*.{json,yaml}" --type fileLength of output: 224
Important: Modify SQL Schema, Do Not Edit Generated Models Directly
- The file
servers/intro_course/db/sqlc/models.gois generated by sqlc (as noted by the file comment), so any manual modifications here could be lost when the models regenerate.- We found SQL schema and query files (
servers/intro_course/db/migration/0002_seat_plan.up.sqlandservers/intro_course/db/query/seat.sql) that define theSeatentity.- Please update those SQL files with any changes regarding the
Seatstructure and then run sqlc to regenerate the models.clients/intro_course_developer_component/src/introCourse/network/mutations/updateSeatPlan.ts (1)
4-19:✅ Verification successful
Function implementation looks good, verify Content-Type header.
The implementation of
updateSeatPlanfollows good practices with proper error handling. However, the Content-Type headerapplication/json-path+jsonis unusual. The standard content type for JSON data isapplication/json.Verify that the server endpoint expects this specific content type:
🏁 Script executed:
#!/bin/bash # Look for other instances of this content type to confirm its usage rg "application/json-path\+json" --type tsLength of output: 3637
Attention Developer: Content-Type Header Usage Confirmed
The implementation of
updateSeatPlanis correct. The custom header'Content-Type': 'application/json-path+json'is used consistently across similar endpoints in the codebase, confirming that it is the expected value for our server.clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatPlanDialog.tsx (1)
19-55: LGTM! Well-structured component with good separation of concernsThe SeatPlanDialog component is well-implemented with a clean structure, proper state management for dialog visibility, and good user experience with the Eye icon for the trigger button.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useUpdateSeats.ts (1)
1-20: Well-structured React Query hook implementationThe useUpdateSeats hook is well-implemented using React Query's useMutation with appropriate query invalidation on success. The overall structure follows best practices for custom hooks.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx (1)
12-29: Well-structured filtering logic with clear separation of concernsThe filtering logic is well-organized with separate conditions for search, assignment status, and Mac availability, making the code readable and maintainable.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/TutorAssignmentFilter.tsx (1)
25-72: LGTM! Well-implemented filter component with intuitive UIThe TutorAssignmentFilter component provides a clean, intuitive interface for filtering seat assignments with a good implementation of the dropdown menu and checkbox items.
servers/intro_course/db/query/seat.sql (1)
12-19: UpdateSeat implementation looks correctThe update query correctly identifies seats by composite key (course_phase_id, seat_name) and updates all relevant fields.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/ResetSeatPlanDialog.tsx (3)
1-12: Import statements are well-organizedThe imports are properly grouped by their purpose - UI components first, followed by icons.
13-18: Props interface is well-definedThe props interface clearly defines the expected properties and their types, which helps with component usage and type safety.
20-57: Dialog implementation follows best practicesThe implementation provides clear user feedback with appropriately disabled buttons during deletion, explicit warning about data loss consequences, and semantic button variants for destructive actions.
One minor suggestion on the DialogDescription - consider using the term "permanently delete" to emphasize the irreversible nature of the action:
<DialogDescription> Are you sure you want to reset the seat plan? This action will - delete all current seats + permanently delete all current seats and{' '} <span className='font-semibold'> all student and tutor seat assignments will be lost </span> . </DialogDescription>clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/SeatAssignmentPage.tsx (2)
64-70: Well-structured loading and error statesGood job consolidating multiple loading and error states into single variables for cleaner conditional rendering.
77-96: Error handling is comprehensive with retry functionalityThe error handling is well implemented with a clear retry mechanism that refreshes all data queries.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatUploader.tsx (1)
94-99: Effective duplicate detection implementation.The duplicate detection using Set comparison is efficient and clearly implemented. Good job!
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useAssignStudents.tsx (2)
73-77: Ensure proper null checks when accessing nested properties.When assigning students, ensure the student ID is properly checked before assignment to prevent potential issues.
if (seatIndex !== -1) { - updatedSeats[seatIndex].assignedStudent = student.participation.student.id ?? null + updatedSeats[seatIndex].assignedStudent = student.participation?.student?.id ?? null }
25-36: Good validation logic for seat assignments.The error checking for seats with tutors and Mac availability is well-implemented and provides clear error messages.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx (2)
55-60: Good reactive state management for assignment status.The effect hook effectively updates the assignment status based on changes in the assigned students count.
186-201: Well-implemented conditional rendering for Mac badge status.The implementation of showing different badges based on the student's Mac ownership status is clean and visually effective.
servers/intro_course/seatPlan/router.go (1)
75-96: updateSeatPlan: Confirm seat existence before update
Though you handle errors in the transaction, consider verifying that each seatName being updated actually exists under the specified coursePhaseID. That way, you can provide a 404 or 400 if someone tries to update seats that don’t belong to this plan.Would you like to verify that each seatName is valid before calling
UpdateSeat? I can generate a script or a code snippet to do so if needed.clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/SeatTutorAssigner.tsx (2)
46-57: handleBatchTutorAssignment: Confirm partial updates
When assigning multiple seats in a batch, any failed seat update can roll back the entire mutation if your backend enforces transaction rules. Ensure you’re handling partial failures or returning an actionable error.Please verify how partial errors are handled on the server side if one seat fails to update while others succeed.
95-125: Error handling logic
Displaying theerrorvia an<Alert>is straightforward. Just ensure yourerrormessages from the server are user-friendly or transformed before showing them directly in the UI.Would you like to unify all seat-related error messages from the server for a more consistent user experience? Let me know if you need a reference snippet.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatMacAssigner.tsx (1)
77-81: Comparing seats for unsaved changes
This comparison is performed by index. If seats ever end up reordered, it might falsely detect or miss changes. If seat order can vary, consider a unique key (e.g., seatName).servers/intro_course/db/sqlc/seat.sql.go (4)
15-29: Well-designed database insertion function for seat plansThe implementation of
CreateSeatPlanuses the PostgreSQLunnestfunction efficiently to insert multiple seat records at once from a string array. This is a good approach that avoids multiple database calls.
41-73: Properly implemented query with proper error handlingThe
GetSeatPlanfunction follows best practices for querying data:
- Properly handles potential errors from the query execution
- Uses deferred closing of rows
- Checks for errors during row iteration and scanning
- Returns appropriate error values
The ordering by seat_name also ensures consistent results.
1-13: Auto-generated file with proper importsThis file is auto-generated by sqlc, which is a good practice for creating type-safe database access code. The imports are appropriate for the functionality implemented.
21-24: Well-structured data models with proper JSON tagsBoth
CreateSeatPlanParamsandUpdateSeatParamsstructs are well-designed with appropriate field types and JSON tags, which helps with API integration. The use of PostgreSQL-specific types likepgtype.Textandpgtype.UUIDfor nullable fields is a good practice.Also applies to: 85-92
...ourse_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx
Outdated
Show resolved
Hide resolved
...rse_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx
Outdated
Show resolved
Hide resolved
...mponent/src/introCourse/pages/SeatAssignment/components/SeatTutorAssigner/SeatTutorTable.tsx
Show resolved
Hide resolved
...urse_developer_component/src/introCourse/pages/SeatAssignment/components/SeatMacAssigner.tsx
Show resolved
Hide resolved
niclasheun
had a problem deploying
to
prompt-dev-vm
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx (1)
24-26: 🛠️ Refactor suggestionAdd null checking to assignment filter condition.
The assignment filter directly accesses properties without null checking. This could cause runtime errors if
seatis undefined or null.const matchesAssignmentFilter = - (seat.assignedTutor && filterOptions.showAssigned) || - (!seat.assignedTutor && filterOptions.showUnassigned) + (seat?.assignedTutor && filterOptions.showAssigned) || + (!seat?.assignedTutor && filterOptions.showUnassigned)
🧹 Nitpick comments (5)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx (4)
12-21: Add type annotations to the escapeCsvField function.The
escapeCsvFieldfunction correctly handles special characters in CSV data, but it lacks type annotations which would improve code readability and maintainability.- function escapeCsvField(field) { + function escapeCsvField(field: unknown): string { // If the field contains commas, quotes, or newlines, wrap it in quotes // and escape any quotes by doubling them. - if (typeof field !== 'string') return field + if (typeof field !== 'string') return String(field) if (field.includes(',') || field.includes('"') || field.includes('\n')) { return `"${field.replace(/"/g, '""')}"` } return field }
49-58: Consider adding error handling to the file download process.The current implementation lacks error handling for potential issues that might occur during the download process. Adding try-catch blocks would make the code more robust.
].join('\n') + try { const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }) const url = URL.createObjectURL(blob) const link = document.createElement('a') link.setAttribute('href', url) link.setAttribute('download', 'seat_assignments.csv') link.style.visibility = 'hidden' document.body.appendChild(link) link.click() document.body.removeChild(link) + // Clean up the URL object + URL.revokeObjectURL(url) + } catch (error) { + console.error('Error downloading CSV file:', error) + // You could also add a user-friendly notification here + } }, [seats, developerWithProfiles, tutors])
24-32: Make the getStudentName function more consistent with error handling.The
getStudentNamefunction returns 'Unassigned' for null IDs and 'Unknown' for IDs that aren't found. Consider using more consistent terminology.const getStudentName = (studentId: string | null) => { - if (!studentId) return 'Unassigned' + if (!studentId) return 'Not assigned' const student = developerWithProfiles.find( (dev) => dev.participation.student.id === studentId, ) return student ? `${student.participation.student.firstName} ${student.participation.student.lastName}` - : 'Unknown' + : 'Not found' }
7-11: Consider adding JSDoc comments to the hook for better documentation.This custom hook would benefit from JSDoc comments that describe what it does, what parameters it takes, and what it returns.
+/** + * A hook that provides functionality to download seat assignments as a CSV file. + * @param seats - Array of seat objects containing assignment information + * @param developerWithProfiles - Array of developers with their profile information + * @param tutors - Array of tutors + * @returns A function that, when called, triggers the download of a CSV file with seat assignments + */ export const useDownloadAssignment = ( seats: Seat[], developerWithProfiles: DeveloperWithProfile[], tutors: Tutor[], ) => {clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx (1)
18-20: Use optional chaining consistently when accessing nested properties.In line 18, you directly access
seat.assignedTutoreven though you check if it's not null. For consistency and safety, use optional chaining.- getTutorName(seat.assignedTutor, tutors) + getTutorName(seat?.assignedTutor, tutors)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
🔇 Additional comments (3)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx (1)
34-47: Well-implemented CSV generation with proper escaping.The implementation of the CSV content generation is correct and properly handles special characters, which is important for data integrity. This is a good example of attention to detail in data exports.
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useGetFilteredSeats.tsx (2)
14-21: Simplify the search filter logic and fix redundant fallbacks.The current search filter implementation contains redundant
|| falsestatements that don't add value to the logic. Additionally, there's inconsistent null checking across the conditions.const matchesSearch = - seat?.seatName?.toLowerCase()?.includes(searchTerm.toLowerCase()) || - false || - (seat?.assignedTutor !== null && - getTutorName(seat.assignedTutor, tutors) - ?.toLowerCase() - ?.includes(searchTerm.toLowerCase())) || - false + (seat?.seatName?.toLowerCase()?.includes(searchTerm.toLowerCase()) || false) || + (seat?.assignedTutor !== null && + getTutorName(seat?.assignedTutor, tutors)?.toLowerCase()?.includes(searchTerm.toLowerCase()) || false)
6-34: Consider adding error handling and edge cases.This hook might benefit from additional error handling for edge cases such as:
- Empty arrays for
seatsortutors- Undefined or invalid filter options
- Performance considerations for large data sets
Are there situations where this hook might receive empty or malformed data? If yes, consider adding appropriate error handling.
| const matchesMacFilter = | ||
| (seat.hasMac && filterOptions.showWithMac) || (!seat.hasMac && filterOptions.showWithoutMac) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checking to Mac filter condition.
Similar to the assignment filter, the Mac filter directly accesses properties without null checking. This could cause runtime errors if seat is undefined or null.
const matchesMacFilter =
- (seat.hasMac && filterOptions.showWithMac) || (!seat.hasMac && filterOptions.showWithoutMac)
+ (seat?.hasMac && filterOptions.showWithMac) || (!seat?.hasMac && filterOptions.showWithoutMac)📝 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.
| const matchesMacFilter = | |
| (seat.hasMac && filterOptions.showWithMac) || (!seat.hasMac && filterOptions.showWithoutMac) | |
| const matchesMacFilter = | |
| (seat?.hasMac && filterOptions.showWithMac) || (!seat?.hasMac && filterOptions.showWithoutMac) |
niclasheun
temporarily deployed
to
prompt-dev-vm
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatUploader.tsx (4)
101-104: Improve error handling type safetyThe error handling in the catch block uses
err: anywhich reduces type safety. Consider using a more specific type or implementing a type guard to ensure safer error handling.- catch (err: any) { - setError(err.message || 'An error occurred while uploading the file.') + catch (err: unknown) { + const errorMessage = err instanceof Error ? err.message : 'An error occurred while uploading the file.' + setError(errorMessage)
86-99: Enhance CSV validation to improve user experienceThe current validation checks for empty files and duplicates, but consider adding validation for the format of seat names and maximum seat count to prevent potential issues later.
try { const seatNames = await readCSVFile(file) if (seatNames.length === 0) { setError('The CSV file is empty or invalid.') setIsUploading(false) return } // Check for duplicate seat names const uniqueCount = new Set(seatNames).size if (uniqueCount !== seatNames.length) { setError('Duplicate seat names detected. Please ensure all seat names are unique.') setIsUploading(false) return } + // Check for maximum seat count limit (if applicable) + const MAX_SEATS = 500; // Adjust as needed + if (seatNames.length > MAX_SEATS) { + setError(`Too many seats. Maximum allowed is ${MAX_SEATS}.`) + setIsUploading(false) + return + } + // Validate seat name format + const invalidNames = seatNames.filter(name => !/^[A-Za-z0-9\-_\s]{1,50}$/.test(name)); + if (invalidNames.length > 0) { + setError(`Invalid seat name format detected. Please use alphanumeric characters, spaces, hyphens and underscores only.`) + setIsUploading(false) + return + } mutation.mutate(seatNames)
185-188: Provide more specific CSV format guidanceThe current description of the CSV format is somewhat vague. Consider providing more specific instructions or a sample format to help users prepare their files correctly.
<p className='text-sm text-muted-foreground'> Upload a CSV file with seat names. Each seat should be on a new line or separated by commas. + For example: "A1,A2,A3" or one seat name per line. + Names should be unique and use only letters, numbers, spaces, hyphens, or underscores. </p>
28-32: Consider consolidating related stateThe component manages several related states separately. Consider consolidating these into a single state object to make state management more predictable and prevent potential synchronization issues.
- const [error, setError] = useState<string | null>(null) - const [isUploading, setIsUploading] = useState(false) - const [isDialogOpen, setIsDialogOpen] = useState(false) - const [selectedFileName, setSelectedFileName] = useState('No file selected') - const [selectionMethod, setSelectionMethod] = useState<'upload' | 'rechnerhalle'>('rechnerhalle') + const [uploadState, setUploadState] = useState({ + error: null as string | null, + isUploading: false, + isDialogOpen: false, + selectedFileName: 'No file selected', + selectionMethod: 'rechnerhalle' as 'upload' | 'rechnerhalle' + }) + + // Helper functions to update specific state properties + const updateUploadState = (updates: Partial<typeof uploadState>) => { + setUploadState(prev => ({ ...prev, ...updates })) + }This would require updating all references to these state variables and setters throughout the component, but would make state transitions more predictable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatUploader.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx(1 hunks)servers/intro_course/seatPlan/router.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
🔇 Additional comments (5)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatUploader/SeatUploader.tsx (1)
23-231: Overall excellent implementation with good structure and error handlingThe SeatUploader component is well-structured with clear separation of concerns between state management, mutations, and UI rendering. The component handles different states appropriately and provides good user feedback. The responsive design considerations and accessibility patterns are commendable.
servers/intro_course/seatPlan/router.go (4)
28-56: Validate seat names more robustly
The uniqueness check is good, but consider additional validation (e.g. checking for empty, too long, or invalid names) to prevent user errors and ensure data consistency.Please verify if further constraints or validation rules are required by the specifications or user stories.
58-73: Seat retrieval logic looks consistent
The endpoint retrieves the seat plan and handles errors appropriately.
75-96: Update seat plan flow looks correct
Parsing seats and callingUpdateSeatPlanfollows a clear pattern. Error handling is consistent with other endpoints.
98-113: Delete endpoint seems fine
Deleting the seat plan is straightforward, and errors are handled properly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx (3)
63-69: Consider optimizing reset implementation.This implementation of
resetAssignmentsonly updates seats that have assigned students, which is efficient for API calls. However, it might cause unexpected behavior if the API expects all seats to be updated at once.const resetAssignments = useCallback(() => { - const updatedSeats = seats - .filter((seat) => seat.assignedStudent != null) - .map((seat) => ({ ...seat, assignedStudent: null })) + const updatedSeats = seats.map(seat => + seat.assignedStudent != null ? { ...seat, assignedStudent: null } : seat + ) mutation.mutate(updatedSeats) setAssignmentStatus('none') }, [seats, mutation])
159-166: Performance optimization for student lookup.The code repeatedly looks up students in the array for each seat. Consider creating a lookup map before rendering to improve performance, especially when dealing with many students.
{assignedStudents > 0 && ( <div className='border rounded-md overflow-hidden mt-4'> <Table> <TableHeader> <TableRow> <TableHead>Seat</TableHead> <TableHead>Mac</TableHead> <TableHead>Assigned Student</TableHead> <TableHead>Student Has Mac</TableHead> </TableRow> </TableHeader> <TableBody> + {(() => { + // Create lookup map for better performance + const studentMap = Object.fromEntries( + developerWithProfiles.map(dev => [dev.participation.student.id, dev]) + ); + + return seats .filter((seat) => seat.assignedStudent) .sort((a, b) => a.seatName.localeCompare(b.seatName)) .map((seat) => { - const student = developerWithProfiles.find( - (dev) => dev.participation.student.id === seat.assignedStudent, - ) + const student = studentMap[seat.assignedStudent ?? '']
118-122: Fix inconsistent color styling in "Fully Assigned" badge.The "Fully Assigned" badge uses
bg-green-500andtext-green-600, which may cause contrast issues. The other badges use consistent styling patterns with-50background and-600text.{assignmentStatus === 'complete' && ( - <Badge variant='outline' className='bg-green-500 text-green-600'> + <Badge variant='outline' className='bg-green-50 text-green-600'> Fully Assigned ({assignedStudents}/{totalStudents}) </Badge> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/ResetSeatAssignmentDialog.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/ResetSeatAssignmentDialog.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
🔇 Additional comments (1)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx (1)
1-213: Well-structured component with thoughtful UI/UX design.The component follows React best practices with proper state management, responsive design, and conditional rendering. I particularly appreciate:
- Clear step indication in the title (Step 4)
- Collapsible UI to save space when not in use
- Thoughtful disable states for buttons
- Status indicators that change appearance based on assignment status
- Well-organized table for showing assignments
niclasheun
temporarily deployed
to
prompt-dev-vm
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx (4)
63-69: Consider optimizing the reset logicThe current implementation only updates seats that have student assignments. While this is efficient for API calls, it might cause inconsistencies if there are other properties that need resetting.
- const resetAssignments = useCallback(() => { - const updatedSeats = seats - .filter((seat) => seat.assignedStudent != null) - .map((seat) => ({ ...seat, assignedStudent: null })) - mutation.mutate(updatedSeats) - setAssignmentStatus('none') - }, [seats, mutation]) + const resetAssignments = useCallback(() => { + const updatedSeats = seats.map((seat) => + seat.assignedStudent != null ? { ...seat, assignedStudent: null } : seat + ) + mutation.mutate(updatedSeats) + setAssignmentStatus('none') + }, [seats, mutation])
139-144: Improve UX for the assign students buttonThe "Assign Students" button is disabled when there are already assignments, requiring users to explicitly reset before reassigning. Consider adding a confirmation dialog when clicking "Assign Students" with existing assignments, to offer a more intuitive workflow.
- <Button - onClick={assignStudents} - disabled={mutation.isPending || assignedStudents > 0} - > - <UserCheck className='mr-2 h-4 w-4' /> - {mutation.isPending ? 'Assigning...' : 'Assign Students'} - </Button> + <Button + onClick={() => { + if (assignedStudents > 0) { + if (window.confirm('This will replace existing assignments. Continue?')) { + resetAssignments(); + setTimeout(assignStudents, 0); + } + } else { + assignStudents(); + } + }} + disabled={mutation.isPending} + > + <UserCheck className='mr-2 h-4 w-4' /> + {mutation.isPending ? 'Assigning...' : assignedStudents > 0 ? 'Reassign Students' : 'Assign Students'} + </Button>
159-204: Add pagination or virtualization for large datasetsFor courses with many students, the table might become very large. Consider adding pagination or virtualization to improve performance when dealing with large numbers of assignments.
You could implement pagination with something like:
const [page, setPage] = useState(0); const rowsPerPage = 10; const paginatedSeats = seats .filter((seat) => seat.assignedStudent) .sort((a, b) => a.seatName.localeCompare(b.seatName)) .slice(page * rowsPerPage, (page + 1) * rowsPerPage); // Then use paginatedSeats in your map function // And add pagination controls below the table
187-200: Enhance accessibility for status badgesThe badge elements for student Mac status lack aria attributes for better screen reader support. Also, the icon in the "Yes" badge doesn't have descriptive text for screen readers.
- {student?.profile?.hasMacBook === true && ( - <Badge variant='outline' className='bg-green-500'> - <Laptop className='h-3 w-3 mr-1' /> - Yes - </Badge> - )} + {student?.profile?.hasMacBook === true && ( + <Badge variant='outline' className='bg-green-500' aria-label="Student has MacBook"> + <Laptop className='h-3 w-3 mr-1' aria-hidden="true" /> + Yes + </Badge> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx(1 hunks)servers/intro_course/main.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- servers/intro_course/main.go
- clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx
🧰 Additional context used
🧬 Code Definitions (1)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx (4)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/interfaces/DeveloperWithProfile.ts (1) (1)
DeveloperWithProfile(4:7)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useUpdateSeats.ts (1) (1)
useUpdateSeats(6:20)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useAssignStudents.tsx (1) (1)
useAssignStudents(6:96)clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/hooks/useDownloadAssignment.tsx (1) (1)
useDownloadAssignment(7:59)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
🔇 Additional comments (3)
clients/intro_course_developer_component/src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx (3)
43-47: State management and derived values look goodThe component properly initializes state for error handling, UI collapse state, and assignment status tracking. The derived values for total and assigned students are calculated efficiently.
55-60: Well-implemented assignment status trackingThe useEffect hook correctly updates the assignment status based on the current state of seat assignments, with appropriate dependency array ensuring it runs only when needed.
1-213: Overall component implementation is well structuredThe SeatStudentAssigner component is well-structured with clear separation of concerns, good state management, and effective use of custom hooks. The UI is intuitive with appropriate feedback mechanisms for user actions and assignment status.
.../src/introCourse/pages/SeatAssignment/components/SeatStudentAssigner/SeatStudentAssigner.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
servers/intro_course/seatPlan/service.go (4)
21-22: Avoid global singleton usage for better testability and flexibility.
While a singleton can be quick to implement, it may couple the service too tightly to the application lifecycle and can complicate testing or limit concurrency strategies.
23-39: Consider adding validation for seat names before creating a seat plan.
An empty or invalid seat name could cause unexpected behavior. Validate or sanitize seatNames before inserting them into the database.
76-83: Include seat name in the error message for easier debugging.
When seat updates fail, help the user pinpoint which seat caused the issue by referencing the seat name in the returned error.- return errors.New("failed to update seat") + return fmt.Errorf("failed to update seat '%s': %w", seatDTO.SeatName, err)
94-107: Handle “seat plan not found” differently from other errors.
Currently, all failures inDeleteSeatPlanare reported as “failed to delete seat plan.” Returning a more specific error or status for non-existent seat plans helps clients and logs differentiate between genuine DB failures and a seat plan that simply doesn’t exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
servers/intro_course/seatPlan/service.go(1 hunks)
🔇 Additional comments (1)
servers/intro_course/seatPlan/service.go (1)
57-92: Rollback on partial failure remains a concern.
If one seat update fails, the entire transaction is rolled back. Please ensure the UI or workflow clarifies which seat(s) triggered the failure, matching your previous open concern about partial updates.
niclasheun
had a problem deploying
to
prompt-dev-vm
GitHub Actions
Failure
This PR introduces the Seat Plan Assignment of the intro course.
Open Todos:
Summary by CodeRabbit