Leave tracker #1390
Leave tracker #1390
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| export const fetchReportsIndex = async (): Promise<ReportsIndexResponse> => ( | ||
| xhrGetAsync<ReportsIndexResponse>(`${EnvironmentConfig.API.V6}/reports`) | ||
| xhrGetAsync<ReportsIndexResponse>(`${EnvironmentConfig.API.V6}/reports/directory`) |
There was a problem hiding this comment.
[❗❗ correctness]
The change in the endpoint from /reports to /reports/directory should be verified to ensure that the new endpoint exists and returns the expected data structure. This change could potentially break the functionality if the endpoint is incorrect or if the response format has changed.
| @@ -0,0 +1 @@ | |||
| export * from './src' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports and make it harder to track what is being exported from this module. Consider explicitly exporting only the necessary modules to improve maintainability and clarity.
|
|
||
| const CalendarApp: FC = () => { | ||
| const { getChildRoutes }: RouterContextData = useContext(routerContext) | ||
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), [getChildRoutes]) |
There was a problem hiding this comment.
[💡 performance]
Using useMemo here might not be necessary unless getChildRoutes is computationally expensive or toolTitle changes frequently. Consider removing useMemo if performance is not a concern.
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), [getChildRoutes]) | ||
|
|
||
| useEffect(() => { | ||
| document.body.classList.add('calendar-app') |
There was a problem hiding this comment.
[maintainability]
Directly manipulating the document.body.classList can lead to side effects that are hard to track. Consider using a state management solution or a CSS-in-JS library to manage class names more predictably.
| domain: AppSubdomain.calendar, | ||
| element: <CalendarApp />, | ||
| id: toolTitle, | ||
| rolesRequired: [UserRole.topcoderStaff, UserRole.administrator], |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding a comment or documentation to clarify the purpose of the rolesRequired array. This will help future developers understand the role-based access control for these routes.
| @@ -0,0 +1,9 @@ | |||
| import { AppSubdomain, EnvironmentConfig } from '~/config' | |||
|
|
|||
| export const rootRoute: string | |||
There was a problem hiding this comment.
[maintainability]
Consider using a function to encapsulate the logic for determining the rootRoute. This can improve maintainability and testability by isolating the logic and making it easier to modify or extend in the future.
| @@ -0,0 +1,116 @@ | |||
| .calendar { | |||
| position: relative; | |||
| background: #ffffff; | |||
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for the background color #ffffff to maintain consistency and make it easier to update the theme in the future.
| cursor: pointer; | ||
| transition: background-color 0.12s ease, transform 0.12s ease, box-shadow 0.12s ease, border-color 0.12s ease; | ||
| font-weight: 700; | ||
| color: #2d2d2d; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for the color #2d2d2d to maintain consistency and make it easier to update the theme in the future.
|
|
||
| .status-leave { | ||
| background: var(--status-leave); | ||
| color: #ffffff; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for the color #ffffff to maintain consistency and make it easier to update the theme in the future.
|
|
||
| const monthDates = useMemo( | ||
| () => getMonthDates(currentDate.getFullYear(), currentDate.getMonth()), | ||
| [currentDate], |
There was a problem hiding this comment.
[performance]
Using currentDate as a dependency in useMemo might lead to unnecessary recalculations if currentDate changes frequently. Consider extracting the year and month outside the component or memoizing them separately to avoid unnecessary re-renders.
|
|
||
| function handleDateClick(event: MouseEvent<HTMLButtonElement>): void { | ||
| const dateKey = event.currentTarget.dataset.dateKey | ||
|
|
There was a problem hiding this comment.
[correctness]
Accessing dataset.dateKey directly assumes that the attribute is always present. Consider adding a default value or handling the case where dateKey might be undefined to prevent potential runtime errors.
| const dateKey = getDateKey(date) | ||
| const status = getStatusForDate(date, leaveDates) | ||
| const isSelected = selectedDates.has(dateKey) | ||
| const statusClass = styles[getStatusColor(status)] |
There was a problem hiding this comment.
[correctness]
The use of dynamic class names with styles[getStatusColor(status)] assumes that getStatusColor always returns a valid key in styles. Consider adding validation or a fallback to handle cases where the status might not map to a defined style.
| align-items: center; | ||
| gap: 8px; | ||
| font-size: 14px; | ||
| color: #4b5563; |
There was a problem hiding this comment.
[maintainability]
Consider using a CSS variable for the color #4b5563 to maintain consistency and ease future updates.
| <div className={styles.legend}> | ||
| {legendItems.map(item => ( | ||
| <div key={item.label} className={styles.item}> | ||
| <span className={classNames(styles.color, styles[item.statusClass])} /> |
There was a problem hiding this comment.
[correctness]
Using styles[item.statusClass] assumes that item.statusClass is a valid key in the styles object. Consider adding a validation or default value to handle cases where item.statusClass might not correspond to a defined style, to prevent potential runtime errors.
| @@ -0,0 +1 @@ | |||
| export * from './CalendarLegend' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports if the CalendarLegend module exports more than intended. Consider explicitly exporting only the necessary components to improve maintainability and avoid potential naming conflicts.
| } | ||
|
|
||
| .contentLayoutOuter { | ||
| margin: $sp-6 auto !important; |
There was a problem hiding this comment.
[maintainability]
Avoid using !important unless absolutely necessary as it can make the CSS harder to maintain and override. Consider refactoring the CSS to achieve the desired effect without !important.
| onPrevMonth: () => void | ||
| } | ||
|
|
||
| export const MonthNavigation: FC<MonthNavigationProps> = (props: MonthNavigationProps) => ( |
There was a problem hiding this comment.
[💡 readability]
Consider destructuring props directly in the function parameters for improved readability and to avoid repetitive props. prefix usage.
| @@ -0,0 +1,151 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
There was a problem hiding this comment.
[maintainability]
Consider using a more specific import path if possible, to avoid potential issues with importing unintended styles from @libs/ui/styles/includes.
|
|
||
| .teamCalendar { | ||
| position: relative; | ||
| background: #ffffff; |
There was a problem hiding this comment.
[design]
Using a hardcoded color #ffffff for the background might lead to issues with theming or dark mode support. Consider using a CSS variable or a theme-based color.
| border: 1px solid var(--calendar-border); | ||
| border-radius: 12px; | ||
| padding: 16px; | ||
| box-shadow: 0 10px 30px rgba(0, 0, 0, 0.04); |
There was a problem hiding this comment.
[💡 maintainability]
The box-shadow property uses a hardcoded RGBA value. Consider using a CSS variable for better maintainability and theming support.
| } | ||
|
|
||
| .weekend { | ||
| background: #f0f7ff; |
There was a problem hiding this comment.
[design]
The background color for weekends is hardcoded. Consider using a CSS variable to allow for easier theming and customization.
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| background: rgba(255, 255, 255, 0.7); |
There was a problem hiding this comment.
[💡 maintainability]
The loading overlay background uses a hardcoded RGBA value. Consider using a CSS variable to improve maintainability and support for different themes.
| const isLoading = props.isLoading | ||
| const teamLeaveDates = props.teamLeaveDates | ||
|
|
||
| const monthDates = useMemo( |
There was a problem hiding this comment.
[performance]
Consider memoizing monthDates with currentDate.getFullYear() and currentDate.getMonth() separately to avoid unnecessary recalculations when only one of them changes.
| ) | ||
| } | ||
|
|
||
| const dateKey = getDateKey(date) |
There was a problem hiding this comment.
[performance]
The find operation on teamLeaveDates could become a performance bottleneck if the array is large. Consider optimizing this by using a more efficient data structure, such as a Map, if possible.
| const leaveEntry = teamLeaveDates.find(item => item.date === dateKey) | ||
| const users = leaveEntry?.usersOnLeave ?? [] | ||
| const sortedUsers = [...users].sort(compareUsersByName) | ||
| const displayedUsers = sortedUsers.slice(0, 10) |
There was a problem hiding this comment.
[performance]
Sorting the users array on every render could be inefficient. Consider memoizing the sorted users list if the users array is large or if sorting is computationally expensive.
| {displayedUsers.length > 0 | ||
| && displayedUsers.map((user, userIndex) => ( | ||
| <div | ||
| key={`${dateKey}-${user.userId}-${userIndex.toString()}`} |
There was a problem hiding this comment.
[correctness]
Using user.userId in the key generation is good for uniqueness, but ensure userId is always defined and unique across users to prevent potential key collisions.
| @@ -0,0 +1,5 @@ | |||
| export * from './Layout' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintentional exports and make it difficult to track what is being exported from this module. Consider explicitly listing the exports to improve maintainability and clarity.
| import type { CalendarContextModel } from '../models' | ||
|
|
||
| export const CalendarContext = createContext<CalendarContextModel>({ | ||
| loginUserInfo: undefined, |
There was a problem hiding this comment.
[correctness]
Consider providing a more meaningful default value for loginUserInfo instead of undefined, if possible. This can help prevent potential runtime errors when accessing properties of loginUserInfo.
| useEffect(() => { | ||
| tokenGetAsync() | ||
| .then((token: TokenModel) => setLoginUserInfo(token)) | ||
| .catch(() => { |
There was a problem hiding this comment.
[maintainability]
Consider logging the error in the .catch block to aid in debugging and monitoring. Silent failures can make it difficult to diagnose issues in production.
| export const SWRConfigProvider: FC<PropsWithChildren> = props => ( | ||
| <SWRConfig | ||
| value={{ | ||
| fetcher: resource => xhrGetAsync(resource), |
There was a problem hiding this comment.
[❗❗ correctness]
Consider handling errors in the fetcher function. Currently, xhrGetAsync is used directly, which might not handle errors gracefully. This could lead to unhandled promise rejections or silent failures.
| @@ -0,0 +1,3 @@ | |||
| export * from './CalendarContext' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports and make it harder to track what is being exported from this module. Consider explicitly exporting only the necessary components or functions to improve maintainability and clarity.
| @@ -0,0 +1,2 @@ | |||
| export * from './useFetchLeaveDates' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports and makes it harder to track what is being exported from this module. Consider explicitly exporting only the necessary functions or variables to improve maintainability and clarity.
| setError(undefined) | ||
|
|
||
| try { | ||
| const response = await fetchUserLeaveDates(startKey, endKey) |
There was a problem hiding this comment.
[correctness]
Consider adding a check for the response format to ensure it matches the expected LeaveDate[] structure before setting it in state. This can prevent potential runtime errors if the API response changes unexpectedly.
| setError(undefined) | ||
|
|
||
| try { | ||
| await setLeaveDatesService(dates, status) |
There was a problem hiding this comment.
[correctness]
The setLeaveDatesService function call does not check for the response or any potential errors before proceeding. Consider validating the response to ensure the update was successful, which can help in handling any unexpected issues.
| const requestKey = buildRequestKey( | ||
| startKey, | ||
| endKey, | ||
| Date.now() |
There was a problem hiding this comment.
[performance]
Using Date.now().toString() as part of the request key could lead to unnecessary cache misses if the function is called in quick succession. Consider using a more stable identifier for the request key if possible.
| } catch (err) { | ||
| if (latestLoadRequestRef.current === requestKey) { | ||
| setError(err) | ||
| handleError(err) |
There was a problem hiding this comment.
[💡 design]
Re-throwing the error after handling it with handleError(err) may lead to duplicate error handling if the caller also handles the error. Ensure that this behavior is intentional.
| @@ -0,0 +1,6 @@ | |||
| export * from './components' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintentional exports and potential naming conflicts. Consider explicitly exporting only the necessary modules to improve maintainability and avoid future issues.
| export type LeaveUpdateStatus = LeaveStatus.AVAILABLE | LeaveStatus.LEAVE | ||
|
|
||
| export interface LeaveDate { | ||
| date: string |
There was a problem hiding this comment.
[correctness]
Consider using a Date object instead of a string for the date field to leverage JavaScript's date manipulation capabilities and reduce potential errors from string parsing.
| } | ||
|
|
||
| export interface TeamLeaveDate { | ||
| date: string |
There was a problem hiding this comment.
[correctness]
Consider using a Date object instead of a string for the date field to leverage JavaScript's date manipulation capabilities and reduce potential errors from string parsing.
| @@ -0,0 +1 @@ | |||
| export * from './leave.service' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports if leave.service contains internal modules not meant for public use. Consider explicitly exporting only the necessary modules to maintain a clear and controlled API surface.
| ): Promise<LeaveDate[]> => { | ||
| const queryString = serializeQuery({ endDate, startDate }) | ||
|
|
||
| return xhrGetAsync(`${EnvironmentConfig.API.V6}/leave/dates${queryString}`) |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the xhrGetAsync call to manage potential network or API errors gracefully.
| export const setLeaveDates = async ( | ||
| dates: string[], | ||
| status: LeaveUpdateStatus, | ||
| ): Promise<void> => xhrPostAsync(`${EnvironmentConfig.API.V6}/leave/dates`, { dates, status }) |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the xhrPostAsync call to manage potential network or API errors gracefully.
| ): Promise<TeamLeaveDate[]> => { | ||
| const queryString = serializeQuery({ endDate, startDate }) | ||
|
|
||
| return xhrGetAsync(`${EnvironmentConfig.API.V6}/leave/team${queryString}`) |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the xhrGetAsync call to manage potential network or API errors gracefully.
| @@ -0,0 +1,37 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
There was a problem hiding this comment.
[maintainability]
Consider using a relative import path instead of an absolute one for @libs/ui/styles/includes to improve maintainability and portability of the code across different environments.
| // App-specific global styles | ||
| } | ||
|
|
||
| .primaryButton { |
There was a problem hiding this comment.
[💡 maintainability]
The .primaryButton class uses a hardcoded color white for the text. Consider using a CSS variable for the text color to maintain consistency and make it easier to update the theme in the future.
| return match?.status ?? LeaveStatus.AVAILABLE | ||
| } | ||
|
|
||
| export const getStatusColor = (status: LeaveStatus): string => ( |
There was a problem hiding this comment.
[correctness]
The fallback for getStatusColor uses statusColorMap[LeaveStatus.AVAILABLE], which assumes LeaveStatus.AVAILABLE will always be present in statusColorMap. Consider adding a check to ensure this key exists or handle the case where it might not, to prevent potential runtime errors.
| statusColorMap[status] ?? statusColorMap[LeaveStatus.AVAILABLE] | ||
| ) | ||
|
|
||
| export const getStatusLabel = (status: LeaveStatus): string => statusLabelMap[status] |
There was a problem hiding this comment.
[correctness]
The getStatusLabel function does not provide a fallback for cases where the status is not found in statusLabelMap. This could lead to undefined being returned if an invalid LeaveStatus is passed. Consider adding a default return value or error handling to improve robustness.
| endOfMonth(currentDate), | ||
| ) | ||
| } catch { | ||
| setActionError('Unable to load leave dates. Please try again.') |
There was a problem hiding this comment.
[maintainability]
Consider logging the error details in the catch block to aid in debugging and provide more context on what went wrong.
| setSelectedDates(new Set()) | ||
| await loadCurrentMonth() | ||
| } catch { | ||
| setActionError('Unable to update leave dates. Please try again.') |
There was a problem hiding this comment.
[maintainability]
Consider logging the error details in the catch block to aid in debugging and provide more context on what went wrong.
| setSelectedDates(new Set()) | ||
| await loadCurrentMonth() | ||
| } catch { | ||
| setActionError('Unable to update leave dates. Please try again.') |
There was a problem hiding this comment.
[maintainability]
Consider logging the error details in the catch block to aid in debugging and provide more context on what went wrong.
| startOfMonth(currentDate), | ||
| endOfMonth(currentDate), | ||
| ) | ||
| } catch { |
There was a problem hiding this comment.
[maintainability]
Catching all exceptions without logging or handling specific error types can make debugging difficult. Consider logging the error or handling specific exceptions to improve maintainability and debugging.
| setCurrentDate(prev => addMonths(prev, 1)) | ||
| }, []) | ||
|
|
||
| const errorMessage = useMemo(() => { |
There was a problem hiding this comment.
[💡 performance]
The useMemo hook is used to memoize the errorMessage, but since it is derived from simple state values, the performance benefit might be negligible. Consider removing useMemo unless profiling shows a performance improvement.
| [myResources], | ||
| ) | ||
|
|
||
| const reviewerLikeResourceIds = useMemo( |
There was a problem hiding this comment.
[💡 performance]
The reviewerLikeResourceIds set is constructed by iterating over multiple other sets and adding their elements. Consider using Set.prototype.add directly in the forEach loop to avoid unnecessary intermediate steps, which could improve performance slightly.
| ) | ||
|
|
||
| // Get role for review flow | ||
| const actionChallengeRole = useMemo<ChallengeRole>(() => { |
There was a problem hiding this comment.
[correctness]
The actionChallengeRole logic has been updated to include reviewerLikeResourceIds. Ensure that this change aligns with the intended business logic, as it alters the conditions under which a 'Reviewer' role is assigned.
New app to track internal staff leave and report to Slack when folks are out on leave.