-
Notifications
You must be signed in to change notification settings - Fork 167
Make Status Site Calendar Functional with users OOO data #1375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,41 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| import { api } from './api'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| type LogsQueryParams = { | ||||||||||||||||||||||||||||||||||||||||||||||
| username?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| type?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| format?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| dev?: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export type LogEntry = { | ||||||||||||||||||||||||||||||||||||||||||||||
| type: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| timestamp?: string | number; | ||||||||||||||||||||||||||||||||||||||||||||||
| from?: string | number; | ||||||||||||||||||||||||||||||||||||||||||||||
| until?: string | number; | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ambiguous date/time field types
Tell me moreWhat is the issue?Date/time fields accept both string and number types without validation or normalization, creating ambiguity in how temporal data should be handled. Why this mattersThis will lead to inconsistent date handling throughout the application, potential runtime errors when consuming code expects a specific format, and difficulty in performing date operations like comparisons or formatting. Suggested change ∙ Feature PreviewDefine consistent types for temporal fields. Either use a single type (e.g., ISO string) or create a union with specific formats: type LogEntry = {
type: string;
timestamp?: string; // ISO 8601 format
from?: string; // ISO 8601 format
until?: string; // ISO 8601 format
taskTitle?: string;
};Or if the API returns mixed formats, add transformation logic in the query to normalize the data. Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||||||||||||||
| taskTitle?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export type LogsResponse = { | ||||||||||||||||||||||||||||||||||||||||||||||
| data: LogEntry[]; | ||||||||||||||||||||||||||||||||||||||||||||||
| message?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export const logsApi = api.injectEndpoints({ | ||||||||||||||||||||||||||||||||||||||||||||||
| endpoints: (builder) => ({ | ||||||||||||||||||||||||||||||||||||||||||||||
| getLogs: builder.query<LogsResponse, LogsQueryParams>({ | ||||||||||||||||||||||||||||||||||||||||||||||
| query: (params) => ({ | ||||||||||||||||||||||||||||||||||||||||||||||
| url: '/logs', | ||||||||||||||||||||||||||||||||||||||||||||||
| params: { | ||||||||||||||||||||||||||||||||||||||||||||||
| username: params.username, | ||||||||||||||||||||||||||||||||||||||||||||||
| type: params.type, | ||||||||||||||||||||||||||||||||||||||||||||||
| format: params.format, | ||||||||||||||||||||||||||||||||||||||||||||||
| dev: params.dev, | ||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary parameter spreading
Tell me moreWhat is the issue?The query method unnecessarily spreads parameters individually when the entire params object could be passed directly, violating the KISS principle. Why this mattersManual parameter spreading creates maintenance overhead as any new parameter would require modifying both the type and the spreading code. This violates DRY and makes the code more prone to errors. Suggested change ∙ Feature Previewquery: (params) => ({
url: '/logs',
params
}),Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||||||||||||||
| providesTags: ['Logs'], | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid sending undefined/falsey query params fetchBaseQuery will serialize params directly; sending undefined/empty values can lead to confusing server behavior. Build the params object conditionally to include only defined keys. Apply this diff: - query: (params) => ({
- url: '/logs',
- params: {
- username: params.username,
- type: params.type,
- format: params.format,
- dev: params.dev,
- },
- }),
+ query: (params) => {
+ const q: Record<string, string | boolean> = {};
+ if (params.username) q.username = params.username;
+ if (params.type) q.type = params.type;
+ if (params.format) q.format = params.format;
+ if (params.dev === true) q.dev = true;
+ return {
+ url: '/logs',
+ params: q,
+ };
+ },📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+25
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick Consider narrowing param and payload types If the backend has known enums (e.g., type: 'REQUEST_CREATED' | ... and format: 'feed' | ...), modeling those as string literal unions will prevent typos at call sites. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||
| overrideExisting: true, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick Drop overrideExisting unless you need to replace existing endpoints Setting overrideExisting: true can accidentally mask endpoint name collisions from other injectors. Prefer the default false unless you’re intentionally overriding. Apply this diff: - overrideExisting: true,
+ // Prefer the default (false) to avoid masking accidental name collisions
+ overrideExisting: false,📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export const { useLazyGetLogsQuery } = logsApi; | ||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,16 +1,41 @@ | ||||||||||||||||||||||||||||||||||
| import { useState, useEffect, ChangeEvent, useRef } from 'react'; | ||||||||||||||||||||||||||||||||||
| import { useRouter } from 'next/router'; | ||||||||||||||||||||||||||||||||||
| import classNames from './UserSearchField.module.scss'; | ||||||||||||||||||||||||||||||||||
| import { useGetAllUsersQuery } from '@/app/services/usersApi'; | ||||||||||||||||||||||||||||||||||
| import { useLazyGetLogsQuery } from '@/app/services/logsApi'; | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick De-duplicate LogEntry type; import from logsApi instead Reuse the exported LogEntry type to avoid divergence. Apply this diff: -import { useLazyGetLogsQuery } from '@/app/services/logsApi';
+import { useLazyGetLogsQuery, type LogEntry } from '@/app/services/logsApi';And remove the local duplicate: -type LogEntry = {
- type: string;
- timestamp?: string | number;
- from?: string | number;
- until?: string | number;
- taskTitle?: string;
-};Also applies to: 8-15 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| import { logs } from '@/constants/calendar'; | ||||||||||||||||||||||||||||||||||
| import { userDataType } from '@/interfaces/user.type'; | ||||||||||||||||||||||||||||||||||
| import { useOutsideAlerter } from '@/hooks/useOutsideAlerter'; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| type LogEntry = { | ||||||||||||||||||||||||||||||||||
| type: string; | ||||||||||||||||||||||||||||||||||
| timestamp?: string | number; | ||||||||||||||||||||||||||||||||||
| from?: string | number; | ||||||||||||||||||||||||||||||||||
| until?: string | number; | ||||||||||||||||||||||||||||||||||
| taskTitle?: string; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| type SearchFieldProps = { | ||||||||||||||||||||||||||||||||||
| onSearchTextSubmitted: (user: userDataType | undefined, data: any) => void; | ||||||||||||||||||||||||||||||||||
| onSearchTextSubmitted: ( | ||||||||||||||||||||||||||||||||||
| user: userDataType | undefined, | ||||||||||||||||||||||||||||||||||
| data: CalendarData[] | ||||||||||||||||||||||||||||||||||
| ) => void; | ||||||||||||||||||||||||||||||||||
| loading: boolean; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| type CalendarData = { | ||||||||||||||||||||||||||||||||||
| userId: string; | ||||||||||||||||||||||||||||||||||
| data: { | ||||||||||||||||||||||||||||||||||
| startTime: number | undefined; | ||||||||||||||||||||||||||||||||||
| endTime: number | undefined; | ||||||||||||||||||||||||||||||||||
| status: string; | ||||||||||||||||||||||||||||||||||
| }[]; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick Constrain status to a specific literal If this component only emits OOO entries, narrow status to 'OOO' to catch accidental values. Apply this diff: - data: {
- startTime: number | undefined;
- endTime: number | undefined;
- status: string;
- }[];
+ data: Array<{
+ startTime: number | undefined;
+ endTime: number | undefined;
+ status: 'OOO';
+ }>;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const SearchField = ({ onSearchTextSubmitted, loading }: SearchFieldProps) => { | ||||||||||||||||||||||||||||||||||
| const router = useRouter(); | ||||||||||||||||||||||||||||||||||
| const isDevMode = router.query.dev === 'true'; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const handleOutsideClick = () => { | ||||||||||||||||||||||||||||||||||
| setDisplayList([]); | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
@@ -22,13 +47,65 @@ const SearchField = ({ onSearchTextSubmitted, loading }: SearchFieldProps) => { | |||||||||||||||||||||||||||||||||
| filterUser(e.target.value); | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const handleOnSubmit = (e: React.FormEvent) => { | ||||||||||||||||||||||||||||||||||
| const toMs = (value?: number | string) => { | ||||||||||||||||||||||||||||||||||
| if (typeof value === 'string') { | ||||||||||||||||||||||||||||||||||
| const parsed = Date.parse(value); | ||||||||||||||||||||||||||||||||||
| return isNaN(parsed) ? undefined : parsed; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if (typeof value !== 'number') return undefined as unknown as number; | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Invalid type casting in toMs function
Tell me moreWhat is the issue?The toMs function returns undefined cast as number when the input is not a number, which creates a type inconsistency. Why this mattersThis type casting will cause runtime errors when the returned value is used in numeric operations or comparisons, as undefined cannot be treated as a number. Suggested change ∙ Feature PreviewReturn undefined directly and update the return type to allow undefined: const toMs = (value?: number | string): number | undefined => {
if (typeof value === 'string') {
const parsed = Date.parse(value);
return isNaN(parsed) ? undefined : parsed;
}
if (typeof value !== 'number') return undefined;
return value >= 1e12 ? value : value * 1000;
};Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||
| return value >= 1e12 ? value : value * 1000; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+50
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Function recreation on every render
Tell me moreWhat is the issue?Timestamp conversion function is defined inside the component causing recreation on every render. Why this mattersFunction recreation on each render cycle wastes memory and CPU cycles, especially problematic when processing arrays of log entries. Suggested change ∙ Feature PreviewMove the const toMs = useCallback((value?: number | string) => {
if (typeof value === 'string') {
const parsed = Date.parse(value);
return isNaN(parsed) ? undefined : parsed;
}
if (typeof value !== 'number') return undefined as unknown as number;
return value >= 1e12 ? value : value * 1000;
}, []);Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit.
Comment on lines
+50
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unexplained Magic Number in Timestamp Conversion
Tell me moreWhat is the issue?The toMs function contains a magic number (1e12) without explanation of its significance in the timestamp conversion logic. Why this mattersFuture developers will need to deduce why this specific threshold was chosen for timestamp conversion, making maintenance more difficult. Suggested change ∙ Feature Previewconst MILLISECONDS_TIMESTAMP_THRESHOLD = 1e12; // Threshold to differentiate between seconds and milliseconds timestamps
const toMs = (value?: number | string) => {
if (typeof value === 'string') {
const parsed = Date.parse(value);
return isNaN(parsed) ? undefined : parsed;
}
if (typeof value !== 'number') return undefined as unknown as number;
return value >= MILLISECONDS_TIMESTAMP_THRESHOLD ? value : value * 1000;
};Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit.
Comment on lines
+50
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix toMs return type and remove unsafe cast Returning Apply this diff: - const toMs = (value?: number | string) => {
- if (typeof value === 'string') {
- const parsed = Date.parse(value);
- return isNaN(parsed) ? undefined : parsed;
- }
- if (typeof value !== 'number') return undefined as unknown as number;
- return value >= 1e12 ? value : value * 1000;
- };
+ const toMs = (value?: number | string): number | undefined => {
+ if (typeof value === 'string') {
+ const parsed = Date.parse(value);
+ return Number.isNaN(parsed) ? undefined : parsed;
+ }
+ if (typeof value !== 'number') return undefined;
+ return value >= 1e12 ? value : value * 1000;
+ };📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const [triggerGetLogs, { isFetching: isLogsFetching }] = | ||||||||||||||||||||||||||||||||||
| useLazyGetLogsQuery(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const handleOnSubmit = async (e: React.FormEvent) => { | ||||||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||||||
| setDisplayList([]); | ||||||||||||||||||||||||||||||||||
| const user = usersList.find( | ||||||||||||||||||||||||||||||||||
| (user: userDataType) => user.username === searchText | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| onSearchTextSubmitted(user, data); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if (!user) { | ||||||||||||||||||||||||||||||||||
| onSearchTextSubmitted(undefined, []); | ||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Feature flag: Use different data sources based on dev mode | ||||||||||||||||||||||||||||||||||
| if (!isDevMode) { | ||||||||||||||||||||||||||||||||||
| // Non-dev mode: Use mock data from constants (original behavior) | ||||||||||||||||||||||||||||||||||
| const userData = data.find((item: any) => item.userId === user.id); | ||||||||||||||||||||||||||||||||||
| onSearchTextSubmitted(user, userData ? [userData] : []); | ||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Dev mode: Fetch real OOO data from logs API | ||||||||||||||||||||||||||||||||||
| const logsResult = await triggerGetLogs({ | ||||||||||||||||||||||||||||||||||
| username: user.username || undefined, | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant undefined fallback for username
Tell me moreWhat is the issue?Unnecessary fallback to undefined when user.username is already optional and could be undefined. Why this mattersThis creates confusing logic where an empty string would be converted to undefined, which may not be the intended behavior and could cause API calls to fail unexpectedly. Suggested change ∙ Feature PreviewPass the username directly without the unnecessary fallback: const logsResult = await triggerGetLogs({
username: user.username,
type: 'REQUEST_CREATED',
format: 'feed',
dev: true,
});Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||
| type: 'REQUEST_CREATED', | ||||||||||||||||||||||||||||||||||
| format: 'feed', | ||||||||||||||||||||||||||||||||||
| dev: true, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+83
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unmaintainable Hard-coded API Parameters
Tell me moreWhat is the issue?Hard-coded string literals 'REQUEST_CREATED' and 'feed' are used directly in the API call without type safety or centralized configuration. Why this mattersChanges to these values would require searching through code, and typos won't be caught by the compiler. Suggested change ∙ Feature Previewconst LOG_TYPES = {
REQUEST_CREATED: 'REQUEST_CREATED'
} as const;
const LOG_FORMATS = {
FEED: 'feed'
} as const;
const logsResult = await triggerGetLogs({
username: user.username || undefined,
type: LOG_TYPES.REQUEST_CREATED,
format: LOG_FORMATS.FEED,
dev: true,
});Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const logsResponse = logsResult.data; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const oooEntries = (logsResponse?.data || []) | ||||||||||||||||||||||||||||||||||
| .filter((log: LogEntry) => log && log.type === 'REQUEST_CREATED') | ||||||||||||||||||||||||||||||||||
| .map((log: LogEntry) => ({ | ||||||||||||||||||||||||||||||||||
| startTime: toMs(log.from), | ||||||||||||||||||||||||||||||||||
| endTime: toMs(log.until), | ||||||||||||||||||||||||||||||||||
| status: 'OOO', | ||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||
| .filter((e) => e.startTime && e.endTime); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
Comment on lines
+92
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Preserve zero epoch and tighten filtering Truthiness check drops 0 timestamps. Use nullish checks instead. Apply this diff: - .filter((e) => e.startTime && e.endTime);
+ .filter((e) => e.startTime != null && e.endTime != null);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| const mapped = [ | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| userId: user.id, | ||||||||||||||||||||||||||||||||||
| data: oooEntries, | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| onSearchTextSubmitted(user, mapped); | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const { data: userData, isError, isLoading } = useGetAllUsersQuery(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.