-
Notifications
You must be signed in to change notification settings - Fork 12
feat: added user preference page issue#41 #45
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
base: main
Are you sure you want to change the base?
feat: added user preference page issue#41 #45
Conversation
… on app load and reusable fern loader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a user preference page that allows users to update their theme and timezone settings, manage preferred projects with pagination, and includes an animated loader that aligns with the Fern UI theme. Key changes include:
- The creation of a new user preference page with form controls, pagination, and project removal features
- Implementation of a provider to handle user preference-related API calls and cookie retrieval
- Addition of a reusable animated loader and updates to header components to support navigation to the preferences page
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
test/component/user-preference.test.tsx | Added tests for rendering, saving preferences, error handling, and actions |
src/utils/cookies.ts | Introduced a helper function to retrieve the user cookie |
src/providers/user-preference-provider.ts | Implemented API calls for updating and fetching user preferences |
src/pages/user-preference/user-preference.tsx | Built the user preference page with settings, pagination, and project actions |
src/pages/user-preference/user-preference-utils.tsx | Added utility functions for project filtering, saving preferences, etc. |
src/pages/user-preference/interface.ts | Defined interfaces for user preferences and project groups |
src/pages/user-preference/index.tsx | Re-exported the user preference page |
src/mocks/mock-preferrencePage-data.ts | Added mock data for user preferences and projects |
src/components/spinner/index.tsx | Implemented the animated loader spinner |
src/components/header/index.tsx | Updated header to include navigation to the preferences page |
src/App.tsx | Integrated user preference routes and loader initialization |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (1)
src/providers/user-preference-provider.ts:4
- [nitpick] The user preference provider imports 'API_URL' from './testrun-provider', which might be confusing. Consider defining a dedicated API_URL constant for user preferences to improve clarity.
import {API_URL} from "./testrun-provider";
@@ -0,0 +1,70 @@ | |||
import { getUserCookie } from "../utils/cookies"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename 'mock-preferrencePage-data.ts' appears to have a spelling error. Consider renaming it to 'mock-preferencePage-data.ts' for consistency.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mrge found 22 issues across 13 files. View them in mrge.io
}); | ||
}); | ||
|
||
test('displays error when preference update fails', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test doesn't verify error display behavior despite its name
fireEvent.click(darkModeSwitch); | ||
|
||
// Verify the setMode function was called | ||
expect(mockSetMode).toHaveBeenCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete assertion for dark mode toggle test
}); | ||
}); | ||
|
||
test('allows changing timezone', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test doesn't actually verify timezone changes
const [currentPage, setCurrentPage] = useState(1); | ||
const [allProjects, setAllProjects] = useState<IProject[]>([]); | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling in async data fetching
const [currentPage, setCurrentPage] = useState(1); | ||
const [allProjects, setAllProjects] = useState<IProject[]>([]); | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No loading state during initial data fetching
|
||
export const userPreferenceProvider = { | ||
updatePreference: async (data: IUserPreference) => { | ||
const response = await axios.put(`${API_URL}/user/preference`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No error handling for API requests - should handle potential network errors, server errors, etc.
@@ -0,0 +1,37 @@ | |||
import axios from "axios"; | |||
import { IUserPreference } from "../pages/user-preference/interface"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing interfaces for API responses - no type safety for returned data
import { message } from 'antd'; | ||
import { userPreferenceProvider } from '../../providers/user-preference-provider'; | ||
import { IProjectGroup, IProject, IUserPreference } from './interface'; | ||
import moment from 'moment-timezone'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using moment.js which is considered legacy and not recommended for new code
import moment from 'moment-timezone'; | ||
|
||
export const fetchPreferredProjects = async (): Promise<IProjectGroup[]> => { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent error handling strategy across functions
return groups.flatMap(group => group.projects); | ||
}; | ||
|
||
export const filterProjectsByGroup = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially confusing function parameter usage
@parthivvv : could you please rebase the code and also address the comments? Now that @sundarok's PR for preferences apis is merged, can we test this out end to end? |
Loader.mov