Team Allocation: Settings Page
#319
Conversation
…location-skill-frontend
WalkthroughThis pull request integrates team allocation functionality into the system. It updates the deployment workflow to include new team allocation parameters, extends environment configurations on both client and shared library sides, and introduces new types for skills, teams, and survey timeframes. A series of network mutation and query functions have been added to support creating, updating, retrieving, and deleting team allocation data. New React components and a survey settings page have been implemented on the client to manage these entities. Server-side changes include initializing a survey module, switching parameter extraction to path parameters, and refining error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SSP as SurveySettingsPage
participant API as API Server
U->>SSP: Navigate to Survey Settings Page
SSP->>API: Request getAllSkills(phaseId)
SSP->>API: Request getAllTeams(phaseId)
SSP->>API: Request getSurveyTimeframe(phaseId)
API-->>SSP: Return Skills, Teams, Timeframe data
SSP->>U: Render SkillSettings, TeamSettings, SurveyTimeframeSettings
Note right of SSP: Handle loading and error states
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (1)
🪧 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 (
|
# Conflicts: # servers/team_allocation/main.go # servers/team_allocation/survey/router.go # servers/team_allocation/survey/service.go # servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go # servers/team_allocation/team/teamDTO/updateTeamRequest.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (38)
clients/team_allocation_component/eslint.config.mjs (1)
1-11: ESLint Configuration Extension Success
This file correctly imports the root ESLint configuration and extends it with an object specifically targeting TypeScript files. The setup is clean and sets a good foundation.Optional: If you plan to enforce TypeScript-specific linting rules, consider populating the
rulesobject with any essential overrides or configurations. Otherwise, if no customizations are needed at this time, you might leave a comment explaining that the empty object is intentional.clients/team_allocation_component/src/team_allocation/interfaces/team.ts (1)
1-4: Type looks good, consider adding JSDoc.The
Teamtype definition is clean and follows good TypeScript practices with appropriate property names and types.Consider adding JSDoc comments to provide more context about the type's purpose and usage:
+/** + * Represents a team entity in the team allocation system. + */ export type Team = { id: string name: string }clients/team_allocation_component/src/index.js (1)
1-3: Consider TypeScript for consistency.The import statements look correct. The dynamic import of bootstrap is a good approach for code splitting.
Consider:
- Converting this file to TypeScript (.ts extension) for consistency with the rest of the project
- Adding a comment explaining the purpose of the dynamic import
import '@/src/index.css' +// Dynamically import bootstrap to enable code splitting and lazy loading import('./bootstrap')clients/team_allocation_component/src/team_allocation/interfaces/skill.ts (1)
1-4: Consider shared base type for Team and Skill.The
Skilltype looks good, but it's identical to theTeamtype.Since both
TeamandSkillhave the same structure, consider creating a shared base type:+/** + * Base type for named entities with unique identifiers + */ +export type NamedEntity = { + id: string + name: string +} + +/** + * Represents a skill in the team allocation system. + */ -export type Skill = { - id: string - name: string -} +export type Skill = NamedEntityThen update the Team type file similarly:
import { NamedEntity } from './namedEntity' /** * Represents a team in the team allocation system. */ export type Team = NamedEntityThis would make it easier to maintain consistent structures and add properties to both types if needed in the future.
clients/eslint.config.mjs (1)
32-32: Component correctly added to ESLint TypeScript configurationThe team_allocation_component has been properly added to the workspaceFolders array, enabling ESLint to resolve its TypeScript configuration.
Consider implementing the TODO comment on line 33 about replacing the manual workspace folder list with dynamic workspace detection, especially as more components are added to the project. This would reduce maintenance overhead when adding new components in the future.
clients/team_allocation_component/src/team_allocation/network/queries/getAllSkills.ts (1)
4-15: Improve error handling for better debuggingThe function is well structured and follows a good pattern for API requests. However, the error handling could be improved to provide more context for debugging.
Consider enhancing the error handling with more context:
export const getAllSkills = async (coursePhaseID: string): Promise<Skill[]> => { try { return ( await teamAllocationAxiosInstance.get( `/team-allocation/api/course_phase/${coursePhaseID}/skill`, ) ).data } catch (err) { - console.error(err) + console.error(`Failed to fetch skills for course phase ${coursePhaseID}:`, err) throw err } }clients/team_allocation_component/src/team_allocation/network/queries/getAllTeams.ts (1)
1-15: Function name and return type should be consistent.The function is named
getAllTeamsbut returnsPromise<Skill[]>. This naming inconsistency could confuse other developers. Consider either:
- Renaming the function to
getAllSkillsto match the return type- Updating the return type to match what the function actually returns (teams)
- Renaming the
Skillinterface toTeamif it actually represents teamsAlso, consider adding validation for the
coursePhaseIDparameter to prevent potential API errors with invalid inputs.-import { Skill } from 'src/team_allocation/interfaces/skill' +import { Skill } from 'src/team_allocation/interfaces/skill' import { teamAllocationAxiosInstance } from '../teamAllocationServerConfig' -export const getAllTeams = async (coursePhaseID: string): Promise<Skill[]> => { +export const getAllTeams = async (coursePhaseID: string): Promise<Skill[]> => { + if (!coursePhaseID) { + throw new Error('Course phase ID is required') + } try { return ( await teamAllocationAxiosInstance.get( `/team-allocation/api/course_phase/${coursePhaseID}/team`, ) ).data } catch (err) { console.error(err) throw err } }clients/team_allocation_component/src/bootstrap.tsx (1)
7-11: Move render inside null check conditionals.If implementing the error handling suggested above, ensure the render logic is placed within the appropriate conditional block.
clients/team_allocation_component/src/team_allocation/network/queries/getSurveyTimeframe.ts (1)
1-15: Consider date conversion for the response data.The function correctly retrieves the survey timeframe data, but there's no date parsing for the response. When working with dates from APIs, they often come as strings and need to be converted to Date objects.
export const getSurveyTimeframe = async (coursePhaseID: string): Promise<SurveyTimeframe> => { try { - return ( - await teamAllocationAxiosInstance.get( - `/team-allocation/api/course_phase/${coursePhaseID}/survey/timeframe`, - ) - ).data + const response = await teamAllocationAxiosInstance.get( + `/team-allocation/api/course_phase/${coursePhaseID}/survey/timeframe`, + ) + + // Parse date strings into Date objects if necessary + const data = response.data + if (data.surveyStart && typeof data.surveyStart === 'string') { + data.surveyStart = new Date(data.surveyStart) + } + if (data.surveyDeadline && typeof data.surveyDeadline === 'string') { + data.surveyDeadline = new Date(data.surveyDeadline) + } + + return data } catch (err) { console.error(err) throw err } }clients/team_allocation_component/src/team_allocation/network/mutations/createTeams.ts (1)
3-21:❓ Verification inconclusive
Verify the Content-Type header and add input validation.
The function uses 'application/json-path+json' as the Content-Type, which is unusual. The standard for JSON is 'application/json'. Also, consider adding input validation for team names before sending the request.
export const createTeams = async (coursePhaseID: string, teamNames: string[]): Promise<void> => { try { + // Validate team names + if (!teamNames || teamNames.length === 0) { + throw new Error('Team names cannot be empty') + } + + // Validate that no team name is empty + if (teamNames.some(name => !name.trim())) { + throw new Error('Team names cannot be empty') + } + const teamsRequest = { teamNames: teamNames, } await teamAllocationAxiosInstance.post( `/team-allocation/api/course_phase/${coursePhaseID}/team`, teamsRequest, { headers: { - 'Content-Type': 'application/json-path+json', + 'Content-Type': 'application/json', }, }, ) } catch (err) { console.error(err) throw err } }
🏁 Script executed:
#!/bin/bash # Search for other instances of 'application/json-path+json' to understand if this is consistently used rg -l "application/json-path\\+json" --type tsLength of output: 2915
Action Required: Confirm Header Update Consistency and Validate Input in createTeams.ts
- The updated diff adds necessary input validation for team names, ensuring that an empty array or blank team names are rejected.
- The Content-Type header is changed from
'application/json-path+json'to the standard'application/json'. However, since multiple files in the codebase still use the non-standard header, please verify whether this change should be applied system-wide or if a targeted change in this mutation is acceptable.export const createTeams = async (coursePhaseID: string, teamNames: string[]): Promise<void> => { try { + // Validate team names + if (!teamNames || teamNames.length === 0) { + throw new Error('Team names cannot be empty') + } + // Validate that no team name is empty + if (teamNames.some(name => !name.trim())) { + throw new Error('Team names cannot be empty') + } + const teamsRequest = { teamNames: teamNames, } await teamAllocationAxiosInstance.post( `/team-allocation/api/course_phase/${coursePhaseID}/team`, teamsRequest, { headers: { - 'Content-Type': 'application/json-path+json', + 'Content-Type': 'application/json', }, }, ) } catch (err) { console.error(err) throw err } }clients/team_allocation_component/src/team_allocation/network/mutations/deleteSkill.ts (1)
3-17:❓ Verification inconclusive
Verify if Content-Type header is necessary for DELETE request.
DELETE requests typically don't include a request body, so setting a Content-Type header might be unnecessary. Additionally, if a Content-Type is needed, the standard for JSON is 'application/json'.
export const deleteSkill = async (coursePhaseID: string, skillID: string): Promise<void> => { try { await teamAllocationAxiosInstance.delete( `/team-allocation/api/course_phase/${coursePhaseID}/skill/${skillID}`, - { - headers: { - 'Content-Type': 'application/json-path+json', - }, - }, ) } catch (err) { console.error(err) throw err } }
🏁 Script executed:
#!/bin/bash # Check if other DELETE requests in the codebase include a Content-Type header rg -l "delete\\(.*headers.*Content-Type" --type tsLength of output: 51
Action: Verify DELETE Request Header Usage
It appears that the DELETE request in this file might not need a Content-Type header since no request payload is sent. If a header is truly required by the API, it should use the standard
'application/json'instead of'application/json-path+json'. However, our initial search for DELETE calls including a Content-Type header didn’t return any matches—please manually verify if other parts of the codebase or backend requirements mandate this header.
- File:
clients/team_allocation_component/src/team_allocation/network/mutations/deleteSkill.ts(Lines 3–17)- Suggestion: Remove the header from the DELETE call unless explicitly needed, or update its value to
'application/json'if required.export const deleteSkill = async (coursePhaseID: string, skillID: string): Promise<void> => { try { await teamAllocationAxiosInstance.delete( `/team-allocation/api/course_phase/${coursePhaseID}/skill/${skillID}`, - { - headers: { - 'Content-Type': 'application/json-path+json', - }, - }, ) } catch (err) { console.error(err) throw err } }clients/team_allocation_component/src/team_allocation/network/mutations/deleteTeam.ts (2)
3-17: The deleteTeam function implementation looks good but has a Content-Type concern.The function correctly implements the DELETE request with proper error handling. However, the Content-Type header 'application/json-path+json' is unusual for a DELETE request since DELETE requests typically don't include a request body.
Consider removing the Content-Type header unless it's specifically required by your API:
export const deleteTeam = async (coursePhaseID: string, teamID: string): Promise<void> => { try { await teamAllocationAxiosInstance.delete( `/team-allocation/api/course_phase/${coursePhaseID}/team/${teamID}`, - { - headers: { - 'Content-Type': 'application/json-path+json', - }, - }, ) } catch (err) { console.error(err) throw err } }
13-16: Consider enhancing error handling.The current error handling logs the error and rethrows it, which is good. However, consider adding more context to help with debugging.
} catch (err) { - console.error(err) + console.error(`Failed to delete team ${teamID} from course phase ${coursePhaseID}:`, err) throw err }clients/team_allocation_component/package.json (1)
6-12: Consider adding test script.The package has scripts for development, linting, and building, but no test script. Adding a test script would improve the development workflow and code quality assurance.
"scripts": { "dev": "webpack serve --open --mode development", "lint": "eslint \"src/**/*.{js,jsx,ts,tsx}\"", "lint:fix": "eslint \"src/**/*.{js,jsx,ts,tsx}\" --fix", "build": "webpack --mode=production --env NODE_ENV=production", - "check-performance": "webpack --mode=production --env NODE_ENV=production --env BUNDLE_SIZE=true" + "check-performance": "webpack --mode=production --env NODE_ENV=production --env BUNDLE_SIZE=true", + "test": "jest" }clients/team_allocation_component/src/team_allocation/network/mutations/createSkills.ts (1)
1-21: LGTM! Consider adding input validation.The implementation is clean and follows a good pattern for API mutations with proper error handling. The function correctly constructs the request payload and uses appropriate headers for the API call.
Consider adding validation for the input parameters to handle edge cases:
export const createSkills = async (coursePhaseID: string, skillNames: string[]): Promise<void> => { try { + if (!coursePhaseID || !skillNames || !Array.isArray(skillNames)) { + throw new Error('Invalid parameters: coursePhaseID must be a non-empty string and skillNames must be an array') + } + + if (skillNames.some(name => !name || typeof name !== 'string')) { + throw new Error('Invalid skillNames: all skill names must be non-empty strings') + } const skillRequest = { skillNames: skillNames, } await teamAllocationAxiosInstance.post( `/team-allocation/api/course_phase/${coursePhaseID}/skill`, skillRequest, { headers: { 'Content-Type': 'application/json-path+json', }, }, ) } catch (err) { console.error(err) throw err } }clients/team_allocation_component/src/team_allocation/network/mutations/updateSkill.ts (1)
1-25: LGTM! Consider adding input validation and destructuring.The implementation follows a good pattern similar to the createSkills mutation, with proper error handling and appropriate request structure.
Consider adding input validation for the parameters and destructuring the newSkillName in the request object:
export const updateSkill = async ( coursePhaseID: string, skillID: string, newSkillName: string, ): Promise<void> => { try { + if (!coursePhaseID || !skillID || !newSkillName) { + throw new Error('Invalid parameters: coursePhaseID, skillID, and newSkillName must be non-empty strings') + } const skillRequest = { - newSkillName: newSkillName, + newSkillName, } await teamAllocationAxiosInstance.put( `/team-allocation/api/course_phase/${coursePhaseID}/skill/${skillID}`, skillRequest, { headers: { 'Content-Type': 'application/json-path+json', }, }, ) } catch (err) { console.error(err) throw err } }servers/team_allocation/survey/router.go (1)
65-71: Changed from query parameters to path parameters.Both courseParticipationID and coursePhaseID are now retrieved from path parameters, which is more consistent with RESTful API practices for resource identifiers.
Since both parameters are being retrieved and parsed the same way, consider creating a helper function to reduce code duplication:
+ // parseUUIDParam parses a UUID parameter from the request path + func parseUUIDParam(c *gin.Context, paramName string) (uuid.UUID, error) { + value, err := uuid.Parse(c.Param(paramName)) + if err != nil { + log.Errorf("Error parsing %s: %v", paramName, err) + } + return value, err + } func submitSurveyResponses(c *gin.Context) { - courseParticipationID, err := uuid.Parse(c.Param("courseParticipationID")) - if err != nil { - log.Error("Error parsing courseParticipationID: ", err) - c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) - return - } - coursePhaseID, err := uuid.Parse(c.Param("coursePhaseID")) - if err != nil { - log.Error("Error parsing coursePhaseID: ", err) - c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) - return - } + courseParticipationID, err := parseUUIDParam(c, "courseParticipationID") + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + coursePhaseID, err := parseUUIDParam(c, "coursePhaseID") + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + }clients/team_allocation_component/src/team_allocation/network/mutations/updateTeam.ts (1)
3-25: Function implementation looks good but consider two improvements.The
updateTeamfunction is well-structured with proper error handling. However:
- The Content-Type header 'application/json-path+json' is unusual. Standard JSON API requests typically use 'application/json'.
- Consider returning the response data in case the caller needs confirmation of success.
export const updateTeam = async ( coursePhaseID: string, teamID: string, newTeamName: string, -): Promise<void> => { +): Promise<any> => { try { const teamRequest = { newTeamName: newTeamName, } - await teamAllocationAxiosInstance.put( + const response = await teamAllocationAxiosInstance.put( `/team-allocation/api/course_phase/${coursePhaseID}/team/${teamID}`, teamRequest, { headers: { - 'Content-Type': 'application/json-path+json', + 'Content-Type': 'application/json', }, }, ) + return response.data } catch (err) { console.error(err) throw err } }clients/team_allocation_component/Dockerfile (1)
1-17: Dockerfile follows good practices but could use minor improvements.The multi-stage build approach is great for keeping the final image small. A few suggestions:
- Consider adding a
.dockerignorefile to exclude unnecessary files from being copied into the container.- You have an unnecessary blank line at line 4 that could be removed.
ARG IMAGE_TAG FROM ghcr.io/ls1intum/prompt2/prompt-clients-base:${IMAGE_TAG} AS core-base - WORKDIR /app/team_allocation_component COPY . ./ RUN yarn install RUN yarn build # Build the final image FROM nginx:stable-alpine COPY --from=core-base /app/team_allocation_component/build /usr/share/nginx/html COPY --from=core-base /app/nginx/nginx.conf /etc/nginx/conf.d/default.conf EXPOSE 80 CMD ["nginx", "-g", "daemon off;"]clients/team_allocation_component/src/OverviewPage.tsx (1)
6-32: Redundant exports detected.The component is exported twice - once as a named export and once as a default export.
Choose either named or default export for consistency:
export const OverviewPage = (): JSX.Element => { // Component implementation } -export default OverviewPage +// Remove redundant exportOr if default export is preferred:
-export const OverviewPage = (): JSX.Element => { +const OverviewPage = (): JSX.Element => { // Component implementation } export default OverviewPageclients/team_allocation_component/tsconfig.json (1)
14-14: Consider enabling noImplicitAny for better type safety.The TypeScript configuration has
strictenabled but explicitly disablesnoImplicitAny, which reduces type safety by allowing variables to be implicitly typed asany.For a new component, it's recommended to enforce strict typing from the start:
- "noImplicitAny": false, + "noImplicitAny": true,clients/core/src/managementConsole/PhaseMapping/PhaseSidebarMapping.tsx (1)
22-22: Naming convention inconsistency in PhaseSidebarMapping.Most keys in the mapping use either camelCase (e.g.,
IntroCourseDeveloper) or snake_case (e.g.,template_component), but'Team Allocation'uses Title Case with a space.Consider following one of the existing naming conventions for consistency:
- 'Team Allocation': TeamAllocationSidebar, + TeamAllocation: TeamAllocationSidebar,Or if the space is required for display purposes, ensure this is consistent with how other phase names are handled throughout the application.
clients/core/webpack.config.ts (1)
36-36: Document port assignment in a central location.As the number of components grows, tracking port assignments becomes increasingly important to avoid conflicts.
Consider documenting port assignments in a central location (README, configuration file, etc.) to make it easier for developers to find available ports for new components.
Also applies to: 104-104
servers/team_allocation/survey/service.go (1)
162-164: Improved error handling for missing timeframe dataThe error handling now correctly distinguishes between a case where no timeframe is set (returning a valid response with
TimeframeSet: false) versus a true error condition. This aligns well with the frontend's need to differentiate these cases.Consider simplifying this with early returns for better readability:
- if err != nil && errors.Is(err, sql.ErrNoRows) { - return surveyDTO.SurveyTimeframe{TimeframeSet: false}, nil - } else if err != nil { + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return surveyDTO.SurveyTimeframe{TimeframeSet: false}, nil + } log.Error("could not get survey timeframe: ", err) return surveyDTO.SurveyTimeframe{}, errors.New("could not get survey timeframe") }clients/team_allocation_component/src/team_allocation/network/mutations/updateSurveyTimeframe.ts (2)
9-12: Simplify object creation with property shorthandYou can use JavaScript's object property shorthand when the property name matches the variable name.
const timeframeUpdateRequest = { - surveyStart: surveyStart, - surveyDeadline: surveyDeadline, + surveyStart, + surveyDeadline, }
3-7: Add client-side validation for timeframe orderingThe function doesn't validate whether
surveyStartis beforesurveyDeadline. Based on the backend implementation inservice.go, the API will reject the request if this condition isn't met. Consider adding client-side validation to provide immediate feedback.export const updateSurveyTimeframe = async ( coursePhaseID: string, surveyStart: Date, surveyDeadline: Date, ): Promise<void> => { + if (surveyStart >= surveyDeadline) { + throw new Error('Survey start must be before survey deadline'); + } try {clients/core/src/managementConsole/PhaseMapping/ExternalSidebars/TeamAllocationSidebar.tsx (1)
12-32: Consider improving error handling and fallback experienceThe implementation of lazy loading with fallback is good, but the error handling could be improved:
- The console warning doesn't include the actual error information
- The fallback UI doesn't explain why the component is unavailable
export const TeamAllocationSidebar = React.lazy(() => import('team_allocation_component/sidebar') .then((module): { default: React.FC<TeamAllocationSidebarProps> } => ({ default: ({ title, rootPath, coursePhaseID }) => { const sidebarElement: SidebarMenuItemProps = module.default || {} return ( <ExternalSidebarComponent title={title} rootPath={rootPath} sidebarElement={sidebarElement} coursePhaseID={coursePhaseID} /> ) }, })) - .catch((): { default: React.FC } => ({ + .catch((error): { default: React.FC } => ({ default: () => { - console.warn('Failed to load team allocation sidebar') + console.warn('Failed to load team allocation sidebar:', error) - return <DisabledSidebarMenuItem title={'Team Allocation Not Available'} /> + return <DisabledSidebarMenuItem + title={'Team Allocation Not Available'} + subtitle={'There was an error loading the sidebar. Please try reloading the page.'} + /> }, })), )clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/TeamSettings.tsx (1)
14-15: Remove or clarify the explanatory commentThe comment "Use the same phaseId context if needed (or adjust as appropriate)" suggests this might be copied code or that there are alternative ways to retrieve the phase ID. Either remove this comment if it's no longer relevant or clarify the intent.
- // Use the same phaseId context if needed (or adjust as appropriate) const phaseId = useParams<{ phaseId: string }>().phaseId ?? ''.github/workflows/build-and-push-clients.yml (1)
152-162: Fix indentation and add newline at end of file.There are two issues with this job definition:
- The indentation on line 153 is inconsistent with other jobs (6 spaces instead of 4)
- The file is missing a newline character at the end (line 162)
Otherwise, the job configuration looks good and follows the pattern of other components.
team-allocation: - needs: clients-base - uses: ls1intum/.github/.github/workflows/[email protected] - with: - image-name: ghcr.io/ls1intum/prompt2/prompt-clients-team-allocation-component - docker-file: clients/team_allocation_component/Dockerfile # Defaults to Dockerfile - docker-context: ./clients/team_allocation_component - build-args: | - "IMAGE_TAG=${{ needs.clients-base.outputs.image_tag }}" - "CORE_HOST=${{ vars.CORE_HOST }}" - secrets: inherit + needs: clients-base + uses: ls1intum/.github/.github/workflows/[email protected] + with: + image-name: ghcr.io/ls1intum/prompt2/prompt-clients-team-allocation-component + docker-file: clients/team_allocation_component/Dockerfile # Defaults to Dockerfile + docker-context: ./clients/team_allocation_component + build-args: | + "IMAGE_TAG=${{ needs.clients-base.outputs.image_tag }}" + "CORE_HOST=${{ vars.CORE_HOST }}" + secrets: inherit +🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 153-153: wrong indentation: expected 4 but found 6
(indentation)
[error] 162-162: no new line character at the end of file
(new-line-at-end-of-file)
clients/team_allocation_component/src/team_allocation/network/teamAllocationServerConfig.ts (2)
5-5: Variable name should match its purpose.The variable name
introCourseServerdoesn't match its content (TEAM_ALLOCATION_HOST), which suggests this may have been copied from an intro course component. Consider renaming it for clarity.-const introCourseServer = env.TEAM_ALLOCATION_HOST || '' +const teamAllocationServer = env.TEAM_ALLOCATION_HOST || ''
7-7: Update variable reference after renaming.If you rename the variable as suggested above, update this reference as well.
-const serverBaseUrl = parseURL(introCourseServer) +const serverBaseUrl = parseURL(teamAllocationServer)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/SurveyTimeframeSettings.tsx (1)
13-140: Good implementation of survey timeframe settings componentThe component is well-structured and follows best practices for React component development:
- Uses proper React hooks for state management
- Implements form validation (start date before end date)
- Handles loading and error states properly
- Disables submission when dates are unchanged or invalid
Consider enhancing accessibility by adding aria-labels to the date inputs:
<Input id='start-date' type='date' value={startDate} onChange={(e) => setStartDate(e.target.value)} className='w-full' + aria-label="Survey start date" required /><Input id='end-date' type='date' value={endDate} onChange={(e) => setEndDate(e.target.value)} className='w-full' + aria-label="Survey end date" required />clients/team_allocation_component/src/team_allocation/pages/SurveySettings/SurveySettingsPage.tsx (2)
16-80: Good implementation of the survey settings pageThe page correctly:
- Fetches required data using React Query
- Handles loading and error states
- Combines the individual settings components
Consider handling errors more granularly, allowing sections with successful data fetching to display even if other sections failed:
- if (isError) { - return <ErrorPage onRetry={refetch} /> - } return ( <> <ManagementPageHeader>Survey Settings</ManagementPageHeader> + {isSurveyTimeframeError ? ( + <Alert variant="destructive"> + <AlertTitle>Failed to load timeframe settings</AlertTitle> + <AlertDescription> + <Button variant="outline" onClick={refetchTimeframe}>Retry</Button> + </AlertDescription> + </Alert> + ) : isSurveyTimeframePending ? ( + <div className="flex justify-center p-4"> + <Loader2 className="h-6 w-6 animate-spin text-primary" /> + </div> + ) : ( <SurveyTimeframeSettings surveyTimeframe={fetchedSurveyTimeframe} /> + )} /* Similar pattern for teams and skills sections */
19-47: Consider refactoring repeated query patternsThe three queries follow an identical pattern. Consider creating a custom hook to reduce repetition:
// Custom hook to simplify query usage function useEntityQuery<T>(key: string, queryFn: () => Promise<T>) { const { phaseId } = useParams<{ phaseId: string }>(); return useQuery<T>({ queryKey: [key, phaseId], queryFn: () => queryFn(phaseId ?? ''), }); } // Usage in component const { data: fetchedSkills, isPending: isSkillsPending, isError: isSkillsError, refetch: refetchSkills, } = useEntityQuery<Skill[]>('team_allocation_skill', getAllSkills);clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/EntitySettings.tsx (2)
50-52: Consider adding error handling for cache invalidation.The
invalidateCachefunction doesn't handle potential errors that might occur during cache invalidation. While React Query typically handles this internally, it's a good practice to add error handling for robustness.const invalidateCache = () => { - queryClient.invalidateQueries({ queryKey }) + try { + queryClient.invalidateQueries({ queryKey }) + } catch (error) { + console.error('Error invalidating cache:', error) + } }
104-110: Use optional chaining for better code safety.When checking for the existence of
editingItem, consider using optional chaining for more concise and safer code.const handleUpdateItem = () => { - if (editingItem && editingItem.name.trim()) { + if (editingItem?.name.trim()) { setUpdateError(null) updateMutation.mutate({ id: editingItem.id, newName: editingItem.name.trim() }) setEditingItem(null) } }🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
clients/team_allocation_component/webpack.config.ts (1)
79-79: Remove TODO comment.There's a TODO comment that should be addressed before merging. The component name should be finalized and the TODO removed.
- name: COMPONENT_NAME, // TODO: rename this to your component name + name: COMPONENT_NAME,servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go (1)
15-21: Consider adding a comment to explain the TimeframeSet usageThe implementation correctly sets
TimeframeSettotruewhen retrieving a timeframe from the database. However, consider adding a comment to clarify the purpose of this field and explain that it will be set tofalsein the service layer when no timeframe is found (as mentioned in the AI summary for theGetSurveyTimeframefunction).func GetSurveyTimeframeDTOFromDBModel(timeframe db.GetSurveyTimeframeRow) SurveyTimeframe { + // TimeframeSet is true when retrieving from DB since the timeframe exists + // It would be set to false in the service layer when no timeframe is found return SurveyTimeframe{ TimeframeSet: true, SurveyStart: timeframe.SurveyStart.Time, SurveyDeadline: timeframe.SurveyDeadline.Time, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clients/team_allocation_component/public/prompt_logo.svgis excluded by!**/*.svgclients/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (55)
.github/workflows/build-and-push-clients.yml(2 hunks).github/workflows/deploy-docker.yml(3 hunks).github/workflows/dev.yml(1 hunks).github/workflows/prod.yml(1 hunks)clients/core/public/env.js(1 hunks)clients/core/public/env.template.js(1 hunks)clients/core/src/managementConsole/PhaseMapping/ExternalRoutes/TeamAllocationRoutes.tsx(1 hunks)clients/core/src/managementConsole/PhaseMapping/ExternalSidebars/TeamAllocationSidebar.tsx(1 hunks)clients/core/src/managementConsole/PhaseMapping/PhaseRouterMapping.tsx(2 hunks)clients/core/src/managementConsole/PhaseMapping/PhaseSidebarMapping.tsx(2 hunks)clients/core/webpack.config.ts(2 hunks)clients/eslint.config.mjs(1 hunks)clients/lerna.json(1 hunks)clients/package.json(1 hunks)clients/shared_library/env.ts(1 hunks)clients/team_allocation_component/.yarnrc.yml(1 hunks)clients/team_allocation_component/Dockerfile(1 hunks)clients/team_allocation_component/eslint.config.mjs(1 hunks)clients/team_allocation_component/package.json(1 hunks)clients/team_allocation_component/public/template.html(1 hunks)clients/team_allocation_component/routes/index.tsx(1 hunks)clients/team_allocation_component/sidebar/index.tsx(1 hunks)clients/team_allocation_component/src/App.tsx(1 hunks)clients/team_allocation_component/src/OverviewPage.tsx(1 hunks)clients/team_allocation_component/src/bootstrap.tsx(1 hunks)clients/team_allocation_component/src/index.js(1 hunks)clients/team_allocation_component/src/team_allocation/interfaces/skill.ts(1 hunks)clients/team_allocation_component/src/team_allocation/interfaces/team.ts(1 hunks)clients/team_allocation_component/src/team_allocation/interfaces/timeframe.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/mutations/createSkills.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/mutations/createTeams.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/mutations/deleteSkill.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/mutations/deleteTeam.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/mutations/updateSkill.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/mutations/updateSurveyTimeframe.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/mutations/updateTeam.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/queries/getAllSkills.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/queries/getAllTeams.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/queries/getSurveyTimeframe.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/teamAllocationServerConfig.ts(1 hunks)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/SurveySettingsPage.tsx(1 hunks)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/DeleteDialog.tsx(1 hunks)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/EntitySettings.tsx(1 hunks)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/SkillSettings.tsx(1 hunks)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/SurveyTimeframeSettings.tsx(1 hunks)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/TeamSettings.tsx(1 hunks)clients/team_allocation_component/tailwind.config.js(1 hunks)clients/team_allocation_component/tsconfig.json(1 hunks)clients/team_allocation_component/webpack.config.ts(1 hunks)docker-compose.prod.yml(1 hunks)servers/team_allocation/main.go(2 hunks)servers/team_allocation/survey/router.go(5 hunks)servers/team_allocation/survey/service.go(2 hunks)servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go(1 hunks)servers/team_allocation/team/teamDTO/updateTeamRequest.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (15)
clients/team_allocation_component/src/bootstrap.tsx (1)
clients/team_allocation_component/src/App.tsx (1)
App(1-8)
clients/team_allocation_component/src/team_allocation/interfaces/timeframe.ts (1)
servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go (1)
SurveyTimeframe(9-13)
servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go (3)
servers/team_allocation/db/sqlc/survey.sql.go (1)
GetSurveyTimeframeRow(107-110)clients/team_allocation_component/src/team_allocation/interfaces/timeframe.ts (1)
SurveyTimeframe(1-5)servers/team_allocation/db/sqlc/models.go (1)
SurveyTimeframe(30-34)
clients/team_allocation_component/src/team_allocation/network/queries/getAllSkills.ts (1)
clients/team_allocation_component/src/team_allocation/interfaces/skill.ts (1)
Skill(1-4)
servers/team_allocation/survey/service.go (3)
servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go (1)
SurveyTimeframe(9-13)clients/team_allocation_component/src/team_allocation/interfaces/timeframe.ts (1)
SurveyTimeframe(1-5)servers/team_allocation/db/sqlc/models.go (1)
SurveyTimeframe(30-34)
clients/core/src/managementConsole/PhaseMapping/PhaseSidebarMapping.tsx (1)
clients/core/src/managementConsole/PhaseMapping/ExternalSidebars/TeamAllocationSidebar.tsx (1)
TeamAllocationSidebar(12-33)
clients/team_allocation_component/src/team_allocation/network/queries/getSurveyTimeframe.ts (2)
clients/team_allocation_component/src/team_allocation/interfaces/timeframe.ts (1)
SurveyTimeframe(1-5)servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go (1)
SurveyTimeframe(9-13)
servers/team_allocation/main.go (1)
servers/team_allocation/survey/main.go (1)
InitSurveyModule(10-16)
clients/team_allocation_component/src/team_allocation/network/teamAllocationServerConfig.ts (1)
clients/shared_library/env.ts (1)
env(20-20)
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/TeamSettings.tsx (2)
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/EntitySettings.tsx (1)
EntitySettings(35-283)clients/team_allocation_component/src/team_allocation/network/mutations/updateTeam.ts (1)
updateTeam(3-25)
clients/team_allocation_component/src/team_allocation/network/queries/getAllTeams.ts (1)
clients/team_allocation_component/src/team_allocation/interfaces/skill.ts (1)
Skill(1-4)
clients/core/src/managementConsole/PhaseMapping/PhaseRouterMapping.tsx (1)
clients/core/src/managementConsole/PhaseMapping/ExternalRoutes/TeamAllocationRoutes.tsx (1)
TeamAllocationRoutes(6-20)
clients/team_allocation_component/routes/index.tsx (2)
clients/team_allocation_component/src/OverviewPage.tsx (1)
OverviewPage(6-30)clients/team_allocation_component/src/team_allocation/pages/SurveySettings/SurveySettingsPage.tsx (1)
SurveySettingsPage(16-80)
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/SurveyTimeframeSettings.tsx (1)
clients/team_allocation_component/src/team_allocation/interfaces/timeframe.ts (1)
SurveyTimeframe(1-5)
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/EntitySettings.tsx (1)
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/DeleteDialog.tsx (1)
DeleteDialog(23-75)
🪛 YAMLlint (1.35.1)
.github/workflows/build-and-push-clients.yml
[warning] 153-153: wrong indentation: expected 4 but found 6
(indentation)
[error] 162-162: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Biome (1.9.4)
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/EntitySettings.tsx
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build-clients / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-team-allocation-component
- GitHub Check: build-clients / devops-challenge / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-devops-challenge-component
- GitHub Check: build-clients / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-assessment-component
- GitHub Check: build-clients / devops-challenge / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-devops-challenge-component
- GitHub Check: build-clients / intro-course-tutor / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-intro-course-tutor-component
- GitHub Check: build-clients / matching / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-matching-component
- GitHub Check: build-clients / intro-course-developer / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-intro-course-developer-component
- GitHub Check: build-clients / interview / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-interview-component
- GitHub Check: build-clients / intro-course-developer / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-intro-course-developer-component
- GitHub Check: build-clients / template / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-template-component
- GitHub Check: build-clients / interview / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-interview-component
- GitHub Check: build-clients / template / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-template-component
- GitHub Check: build-clients / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-core
- GitHub Check: build-clients / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-core
🔇 Additional comments (57)
clients/core/public/env.js (1)
5-5:❓ Verification inconclusive
Environment variable added correctly.
The new
TEAM_ALLOCATION_HOSTenvironment variable follows the project's naming conventions and uses a consistent localhost port pattern.Ensure that you've also updated corresponding environment configuration files such as:
env.template.js(for production)- Any Docker or deployment configuration files
- Documentation that lists available environment variables
🏁 Script executed:
#!/bin/bash # Verify that env.template.js has been updated with the new variable grep -n "TEAM_ALLOCATION_HOST" clients/core/public/env.template.jsLength of output: 119
Action Required: Confirm Docker/Deployment and Documentation Updates
- The new environment variable
TEAM_ALLOCATION_HOSThas been added correctly inclients/core/public/env.jsas'http://localhost:8083'.- Verification shows that
env.template.js(line 9) also includesTEAM_ALLOCATION_HOST: '$TEAM_ALLOCATION_HOST',which aligns with production requirements.- Please ensure that any related Docker or deployment configuration files and the project documentation (listing available environment variables) are updated accordingly.
clients/team_allocation_component/.yarnrc.yml (1)
1-2: Configuration looks good!This Yarn configuration properly sets the node linker to use the traditional node_modules directory structure, which is consistent with the project's dependency management approach.
clients/core/public/env.template.js (1)
9-9: Environment variable properly addedThe new
TEAM_ALLOCATION_HOSTvariable follows the established naming convention and format of other host variables in the file. This is necessary for the team allocation component to make HTTP requests to its backend server.clients/lerna.json (1)
13-14: Package correctly added to Lerna configurationThe team_allocation_component has been properly added to the packages array, which will allow Lerna to manage it alongside other components in the monorepo.
servers/team_allocation/team/teamDTO/updateTeamRequest.go (1)
4-4: Fixed JSON field name casing for better frontend-backend consistencyThe change from
json:"newteamName"tojson:"newTeamName"properly aligns the JSON field with camelCase convention used in JavaScript/TypeScript, improving consistency between frontend and backend.If this API was previously in use, verify that all frontend code has been updated to use the new camelCase field name to avoid potential issues with backward compatibility.
.github/workflows/dev.yml (1)
50-50: Integration of Team Allocation component successfully implementedThe addition of the
team_allocation_image_tagparameter follows the established pattern for component deployment in this workflow. This change properly integrates the new Team Allocation component into the deployment process.clients/shared_library/env.ts (1)
10-10: Environment type correctly extendedThe addition of
TEAM_ALLOCATION_HOSTto theEnvTypeis consistent with the existing environment variable pattern and necessary for proper configuration of the Team Allocation component.clients/team_allocation_component/src/App.tsx (1)
1-10: LGTM! Simple placeholder component implemented correctly.The App component follows React best practices with proper TypeScript typing and correct use of fragments. Both named and default exports are provided which offers flexibility for importing.
clients/team_allocation_component/tailwind.config.js (1)
1-5: LGTM! Tailwind configuration extends shared config properly.The configuration correctly extends the shared Tailwind config and specifies appropriate content paths to scan for class names in both the local component files and shared library components.
clients/core/src/managementConsole/PhaseMapping/ExternalRoutes/TeamAllocationRoutes.tsx (1)
6-20: Well-structured lazy loading implementation for team allocation routes.The component correctly implements React.lazy for dynamically loading the team allocation routes, with proper error handling and fallback UI when loading fails. This approach optimizes performance by loading the component only when needed.
clients/team_allocation_component/routes/index.tsx (2)
6-17: Routes are well-defined with appropriate permissions.The routes array correctly defines two routes:
- Root path for the overview page with no permission requirements
- Settings path restricted to admin and lecturer roles only
This ensures that only authorized users can access the survey settings.
1-1:❓ Verification inconclusive
Import path might be incorrect.
The import path references
template_component/src/OverviewPage, but based on the code snippets, the component appears to be defined inclients/team_allocation_component/src/OverviewPage.tsx.Verify if this import path is correct. If the OverviewPage is indeed within the team_allocation_component package, the import path should be relative or use proper module resolution.
🏁 Script executed:
#!/bin/bash # Check if OverviewPage.tsx exists in both locations echo "Checking for template_component/src/OverviewPage.tsx" fd "OverviewPage.tsx" --full-path "template_component/src" echo "Checking for team_allocation_component/src/OverviewPage.tsx" fd "OverviewPage.tsx" --full-path "team_allocation_component/src"Length of output: 591
Action Required: Confirm and Correct Import Path
The automated search was inconclusive due to invalid directory paths. Please manually verify the location of
OverviewPage.tsx:
- If the file is located at
clients/team_allocation_component/src/OverviewPage.tsx, update the import inclients/team_allocation_component/routes/index.tsxto use a relative path.- Otherwise, ensure that the module resolution accurately reflects the file's location in your project structure.
clients/team_allocation_component/package.json (1)
1-18: Package configuration follows best practices.The package.json file is well structured with appropriate scripts for development, linting, building, and performance checking. The minimal dependencies and clear package configuration indicate good package management practices.
clients/package.json (1)
15-16: LGTM! Correctly adds team_allocation_component to workspaces.The addition of the team_allocation_component to the workspaces configuration ensures it will be properly managed within the monorepo structure.
servers/team_allocation/survey/router.go (3)
47-47: Changed from query parameter to path parameter.Same change pattern - using path parameter instead of query parameter for courseParticipationID.
95-95: Changed from query parameter to path parameter.Consistent application of the change from query parameter to path parameter for coursePhaseID.
29-29:✅ Verification successful
Changed from query parameter to path parameter.
The implementation has been modified to get coursePhaseID from the URL path parameter instead of a query parameter, which aligns with RESTful API design best practices for resource identifiers.
Ensure that the route definitions in the setupSurveyRouter function have been updated to include the path parameter:
🏁 Script executed:
#!/bin/bash # Verify that the router setup includes path parameters grep -r "coursePhaseID" --include="*.go" servers/team_allocation/Length of output: 8624
Verified Update: Correct Usage of Path Parameter for coursePhaseID
The change to extract coursePhaseID from the URL path using
c.Param("coursePhaseID")is confirmed. The router setup inservers/team_allocation/main.gocorrectly defines the route using:coursePhaseID, ensuring alignment with RESTful design practices.
- The grep output confirms that path parameters are used consistently across the codebase.
- Note: There are still some inline comments in
servers/team_allocation/survey/router.goreferring to "query parameter" expectations. Updating these comments for clarity is recommended.clients/core/src/managementConsole/PhaseMapping/PhaseRouterMapping.tsx (2)
12-12: Import looks good.The import of
TeamAllocationRoutesaligns with the existing pattern of importing external route components for different phases.
23-23: New route mapping implemented correctly.The mapping of 'Team Allocation' to
TeamAllocationRoutesfollows the established pattern in the codebase. Checked the implementation ofTeamAllocationRoutesfrom the provided code snippets, and it correctly uses lazy loading for dynamic importing.servers/team_allocation/main.go (2)
15-15: Import for survey module looks good.The import of the survey module follows the established pattern for other modules in this application.
91-91: Survey module initialization is properly implemented.The initialization of the survey module follows the same pattern as the skills and teams modules. Based on the provided context,
InitSurveyModuleis well-documented and sets up the necessary router and service components.clients/team_allocation_component/tsconfig.json (2)
18-18: Path alias points to shared library outside component directory.The path alias is correctly configured, but it's worth noting that it references a shared library outside the component's directory, which establishes a dependency on the parent directory structure.
This is a valid approach for sharing code across components, but ensure the shared library's location remains stable to avoid breaking changes.
21-21: Verify included files for compilation.The
includearray specifies which files should be included in the TypeScript compilation process.The configuration includes TypeScript files, React components, and configuration files that might contain type definitions. This looks appropriate for a React component.
clients/core/src/managementConsole/PhaseMapping/PhaseSidebarMapping.tsx (1)
9-9: Import for TeamAllocationSidebar looks good.The import statement follows the same pattern as other sidebar components.
clients/core/webpack.config.ts (3)
36-36: New team allocation URL follows existing pattern.The URL configuration for the team allocation component correctly follows the same pattern as other components, using port 3008 for local development and a relative path for production.
104-104: Module Federation remote entry correctly configured.The team allocation component is properly added to the ModuleFederationPlugin's remotes configuration, following the same pattern as other components with timestamp-based cache busting.
36-104:❓ Verification inconclusive
Verify team allocation component port availability.
Port 3008 is assigned for the team allocation component in development mode. While this follows the sequential pattern of other components, ensure this port is available and not conflicting with other services.
Run the following script to check if port 3008 is already in use by another service:
🏁 Script executed:
#!/bin/bash # Check if port 3008 is already in use if command -v nc &> /dev/null; then if nc -z localhost 3008 &> /dev/null; then echo "WARNING: Port 3008 is already in use by another service" exit 1 else echo "Port 3008 is available" fi elif command -v lsof &> /dev/null; then if lsof -i:3008 &> /dev/null; then echo "WARNING: Port 3008 is already in use by another service" exit 1 else echo "Port 3008 is available" fi else echo "Unable to check port availability - install nc or lsof" fiLength of output: 152
Action: Manually Verify Port 3008 Availability for Team Allocation Component
The current configuration assigns port 3008 in development mode for the team-allocation component. However, the automated check script was unable to determine port availability because neither
ncnorlsofis installed in the environment. Please ensure that:
- Either install
nc(netcat) orlsofto enable automated port checking, or manually verify that port 3008 is free and not conflicting with another service.- No other service is occupying port 3008 to avoid potential conflicts during development.
clients/team_allocation_component/src/team_allocation/network/mutations/updateSurveyTimeframe.ts (1)
18-18:❓ Verification inconclusive
Verify Content-Type header value
The Content-Type
application/json-path+jsonseems unusual. The standard for JSON API requests is typicallyapplication/json. Verify that the server expects this specific content type.
🏁 Script executed:
#!/bin/bash # Search for other Content-Type headers in the codebase to verify consistency echo "Searching for Content-Type headers in the codebase..." rg -A 1 -B 1 "Content-Type.*application/json" --glob "*.ts" --glob "*.tsx"Length of output: 12193
Attention: Confirm Custom Content-Type Handling
I noticed that
application/json-path+jsonis used consistently across many mutation endpoints (including inclients/team_allocation_component/src/team_allocation/network/mutations/updateSurveyTimeframe.ts). Given its uniform usage throughout the codebase, it appears to be an intentional design choice. However, please verify that the backend service is configured to expect and correctly process this custom media type. If this is the intended behavior, consider documenting the header's purpose to avoid future confusion.clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/TeamSettings.tsx (1)
1-32: Clean component implementation with good structureThe
TeamSettingscomponent follows good practices by:
- Using proper typing for props
- Leveraging route parameters for context
- Providing clear titles and descriptions
- Following a consistent pattern for entity management
The component looks well-structured and properly integrates with the mutation functions.
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/SkillSettings.tsx (1)
1-34: Clean and well-structured component implementation!The
SkillSettingscomponent is well-implemented, using the genericEntitySettingscomponent for consistent CRUD operations on skills. The component correctly retrieves thephaseIdfrom URL parameters and passes appropriate functions for creating, updating, and deleting skills..github/workflows/deploy-docker.yml (3)
42-44: LGTM! Properly added team allocation image tag input.The new input parameter is correctly defined following the same pattern as other component image tags.
143-143: LGTM! Added team allocation host environment variable.Correctly added the TEAM_ALLOCATION_HOST environment variable to the .env.prod file.
177-177: LGTM! Added team allocation image tag environment variable.Correctly added the TEAM_ALLOCATION_IMAGE_TAG environment variable to the .env.prod file.
.github/workflows/build-and-push-clients.yml (1)
33-35: LGTM! Added team allocation image tag output.Correctly defined the new output variable following the same pattern as other component image tags.
clients/team_allocation_component/src/team_allocation/network/teamAllocationServerConfig.ts (1)
9-27: LGTM! Well-structured authentication configuration.The Axios instance is properly configured with the base URL and authentication interceptor. The Patch interface is well-defined for standardized PATCH operations.
docker-compose.prod.yml (1)
305-324: The team allocation service is correctly configuredThe new
client-team-allocation-componentservice follows the same pattern as other client components in the file, with proper configuration for Docker image, container name, environment variables, networking, and Traefik routing rules.clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/SurveyTimeframeSettings.tsx (2)
34-44: Robust error handling for mutationThe mutation implementation correctly invalidates the query cache on success and sets appropriate error messages on failure.
46-57: Good validation in form submission handlerThe form submission handler properly prevents the default form behavior, validates that start date is before end date, and provides clear error messaging.
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/DeleteDialog.tsx (1)
16-75: Well-designed reusable delete confirmation dialogThis component follows good practices for reusable UI components:
- Takes appropriate props for customization
- Handles loading state with visual feedback
- Provides clear user warnings about the permanent nature of deletion
- Uses semantic UI components from the design system
clients/team_allocation_component/src/team_allocation/pages/SurveySettings/components/EntitySettings.tsx (8)
13-16: Good interface definition.The
Entityinterface provides a clear contract for the generic component with essential properties. This enables type-safe reuse across different entity types.
18-33: Well-structured component props interface.The
EntitySettingsPropsinterface is comprehensive and well-documented with comments explaining each prop's purpose. The generic type parameter extends the base Entity interface, allowing for type-safe operations while maintaining flexibility.
54-63: Good mutation setup with proper success/error handling.The create mutation is well-structured with appropriate success and error handlers that update the UI state accordingly. The error message is user-friendly and includes the entity type dynamically.
129-130: Good pluralization handling.The dynamic pluralization based on items count is a nice UX detail that improves readability.
138-177: Well-implemented form for adding new entities.The form implementation follows best practices:
- Prevents default form submission
- Disables the button during pending operations
- Shows loading state
- Displays clear error messages
- Uses proper form validation
179-273: Robust entity list with edit/delete functionality.The implementation of the entity list is comprehensive with:
- Empty state handling
- Edit mode with form submission
- Delete confirmation dialog
- Loading states during operations
- Error handling for each operation
261-266: Good implementation of the delete dialog.The DeleteDialog component is properly integrated with appropriate props and disabled state during deletion operations. This ensures a good user experience by preventing multiple deletion attempts.
96-102:❓ Verification inconclusive
Ensure input validation is comprehensive.
While the function checks for non-empty strings with
trim(), consider adding more validation logic if there are additional requirements for entity names (e.g., length limits, character restrictions).
🏁 Script executed:
#!/bin/bash # Check if there are any validation rules defined elsewhere in the codebase rg -A 3 -B 3 "validation.*name" --glob "*.ts*"Length of output: 48
Attention: Revisit Input Validation Requirements
The current validation in
EntitySettings.tsx(lines 96-102) only trims the input and checks for non-empty strings. No additional validation rules (e.g., maximum length, permitted characters) were found elsewhere in the codebase. Please verify whether there are specific business rules for entity names that would require more comprehensive validation. If so, consider enhancing the validation logic accordingly.clients/team_allocation_component/webpack.config.ts (7)
15-16: Good component-specific configuration.Clearly defining the component name and port as constants at the top of the file makes it easy to identify and modify these values if needed.
21-25: Well-structured configuration function.Using a function for configuration allows for dynamic settings based on environment variables, which is a good practice.
27-44: Comprehensive development server configuration.The development server is well-configured with appropriate settings for hot reloading, history API fallback, and progress reporting.
45-65: Well-organized module rules for TypeScript and CSS.The module rules are clearly defined with appropriate loaders for TypeScript and CSS, including PostCSS for Tailwind integration.
71-76: Good alias configuration.The alias for the shared library simplifies imports and improves maintainability.
78-98: Well-configured ModuleFederationPlugin.The ModuleFederationPlugin is correctly set up to expose routes and sidebar modules while sharing dependencies as singletons. This ensures consistent behavior across micro-frontends.
118-120: Good cache configuration.Using filesystem caching improves build performance, especially for large projects.
servers/team_allocation/survey/surveyDTO/setSurveyTimeframeRequest.go (1)
9-13: Good addition of timeframe status indicator fieldThe
TimeframeSetfield is a good addition to theSurveyTimeframestruct that enables the frontend to know whether a timeframe has been configured. This aligns well with the settings page functionality described in the PR objectives..github/workflows/prod.yml (1)
27-27: LGTM: Successfully integrated team allocation image tagThe addition of the
team_allocation_image_tagparameter follows the existing pattern of other image tags in the workflow. This properly connects the client build outputs to the deployment process.clients/team_allocation_component/src/team_allocation/interfaces/timeframe.ts (1)
1-5: LGTM: Well-structured TypeScript interfaceThe
SurveyTimeframetype is correctly defined and properly matches the backend DTO structure from the Go code. This ensures type consistency between frontend and backend, following best practices for type safety.
# Conflicts: # clients/team_allocation_component/public/template.html # clients/team_allocation_component/routes/index.tsx # clients/team_allocation_component/sidebar/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clients/team_allocation_component/src/team_allocation/network/queries/getAllSkills.ts (2)
11-14: Consider adding more specific error handling.While the current error handling approach works, you might want to differentiate between different types of errors (network errors, server errors, etc.) to provide more specific feedback to the user.
} catch (err) { console.error(err) + // Consider more granular error handling + if (axios.isAxiosError(err)) { + if (err.response?.status === 404) { + console.error('Skills not found for this course phase') + } else if (err.response?.status >= 500) { + console.error('Server error when fetching skills') + } + } throw err }
4-10: Consider adding validation for coursePhaseID parameter.The function doesn't validate if coursePhaseID is a valid string (e.g., not empty or undefined) before making the API call, which could lead to unnecessary network requests.
export const getAllSkills = async (coursePhaseID: string): Promise<Skill[]> => { + if (!coursePhaseID || coursePhaseID.trim() === '') { + throw new Error('Invalid coursePhaseID provided') + } try { return ( await teamAllocationAxiosInstance.get( `/team-allocation/api/course_phase/${coursePhaseID}/skill`, ) ).data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
clients/team_allocation_component/public/template.html(0 hunks)clients/team_allocation_component/routes/index.tsx(2 hunks)clients/team_allocation_component/sidebar/index.tsx(1 hunks)clients/team_allocation_component/src/team_allocation/network/queries/getAllSkills.ts(1 hunks)clients/team_allocation_component/src/team_allocation/network/queries/getAllTeams.ts(1 hunks)
💤 Files with no reviewable changes (1)
- clients/team_allocation_component/public/template.html
🚧 Files skipped from review as they are similar to previous changes (3)
- clients/team_allocation_component/routes/index.tsx
- clients/team_allocation_component/src/team_allocation/network/queries/getAllTeams.ts
- clients/team_allocation_component/sidebar/index.tsx
🧰 Additional context used
🧬 Code Definitions (1)
clients/team_allocation_component/src/team_allocation/network/queries/getAllSkills.ts (1)
clients/team_allocation_component/src/team_allocation/interfaces/skill.ts (1)
Skill(1-4)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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 / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- 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
🔇 Additional comments (1)
clients/team_allocation_component/src/team_allocation/network/queries/getAllSkills.ts (1)
1-15: Well-structured API query function with good error handling.This function follows TypeScript best practices with proper typing and error handling. The implementation correctly retrieves skills data from the backend using the appropriate endpoint structure.
niclasheun
had a problem deploying
to
prompt-dev-vm
GitHub Actions
Failure
niclasheun
had a problem deploying
to
prompt-dev-vm
GitHub Actions
Failure
|
Nice, grad während ich hier reviewe 😉 |
Summary
Was unproductive writing master thesis -> got depressed -> needed to be productive -> wrote the whole team survey setup.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes