-
Notifications
You must be signed in to change notification settings - Fork 57
feat: integrate Redux for job management and add selected category de… #5
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?
Conversation
…tail page - Added Redux actions and reducers for job fetching. - Created a Redux store and integrated it into the application. - Updated PopularCategories component to fetch and display job categories dynamically. - Implemented SelectedCategoryDetail component to show jobs based on selected category. - Refactored Jobs component to utilize Redux for job data management. - Updated App component to include new routes for selected category detail. - Added necessary dependencies for Redux and Axios in package.json.
|
Thankyou @pushmeets i will review your code and merge if it's fine ! |
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 integrates Redux for job data management and introduces category-based job navigation.
- Adds Redux store, actions, and reducers for fetching jobs, and wires Provider into the app.
- Refactors Jobs and PopularCategories to use Redux; adds SelectedCategoryDetail route and component.
- Updates backend Cloudinary import and package scripts; adds frontend dependencies.
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/main.jsx | Wraps the app with Redux Provider to enable global state. |
| frontend/src/components/Job/SelectedCategoryDetail.jsx | New component showing jobs filtered by selected category via Redux state. |
| frontend/src/components/Job/Jobs.jsx | Refactors job fetching to Redux, introduces loading UI. |
| frontend/src/components/Home/PopularCategories.jsx | Dynamically builds categories from jobs and navigates to category detail route. |
| frontend/src/Redux/store.js | Creates Redux store with jobs reducer. |
| frontend/src/Redux/reducers.js | Adds jobs reducer handling request/success/failure. |
| frontend/src/Redux/actions.js | Adds thunk to fetch jobs from API. |
| frontend/src/App.jsx | Adds route for category detail page. |
| frontend/package.json | Adds Redux-related deps (and other packages). |
| backend/server.js | Updates Cloudinary import/config to v2-style API. |
| backend/package.json | Switches scripts to nodemon and bumps cloudinary version. |
Files not reviewed (1)
- backend/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
frontend/src/components/Home/PopularCategories.jsx:1
- These icon imports are currently unused after switching to dynamic categories. Remove unused imports to reduce bundle size and avoid linter warnings.
import React, { useEffect } from "react";
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| const handleClick = (category) => { | ||
| navigate(`/jobs/${category}`); | ||
| console.log(id); |
Copilot
AI
Oct 7, 2025
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.
id is not defined; this will throw a ReferenceError on click. Remove the log or change it to console.log(category).
| console.log(id); | |
| console.log(category); |
| <div className="icon">{element.icon}</div> | ||
| <div | ||
| className="card" | ||
| key={element.id} |
Copilot
AI
Oct 7, 2025
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.
fetchEachCategory is an array of strings, so element.id is undefined and will produce unstable/missing keys. Use the string itself or an index as the key, e.g., key={element}.
| key={element.id} | |
| key={element} |
| useEffect(() => { | ||
| dispatch(getJobs()); | ||
| }, []); | ||
| console.log(jobs); |
Copilot
AI
Oct 7, 2025
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.
Remove debug logging or guard it behind a development flag to keep console output clean in production.
| console.log(jobs); | |
| if (process.env.NODE_ENV === 'development') { | |
| console.log(jobs); | |
| } |
| <div | ||
| className="card" | ||
| key={element.id} | ||
| onClick={() => { | ||
| handleClick(element); | ||
| }} | ||
| > | ||
| <p>{element}</p> |
Copilot
AI
Oct 7, 2025
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.
Clickable divs are not keyboard accessible. Use a semantic button/Link or add role='button', tabIndex={0}, and an onKeyDown handler for Enter/Space to support keyboard users.
| @@ -1,46 +1,72 @@ | |||
| import React, { useContext, useEffect, useState } from "react"; | |||
| import axios from "axios"; | |||
Copilot
AI
Oct 7, 2025
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.
axios is no longer used after refactoring to Redux; remove the unused import to satisfy linters and reduce bundle size.
| import axios from "axios"; |
|
|
||
| if (!isAuthorized) { | ||
| navigateTo("/"); | ||
| } | ||
| }, []); | ||
| if (!isAuthorized) { | ||
| navigateTo("/"); | ||
| } | ||
| }, [dispatch]); |
Copilot
AI
Oct 7, 2025
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 effect depends on isAuthorized and navigateTo but they aren’t included in the dependency array, which can cause stale navigation behavior and ESLint warnings. Either split into two effects or include all dependencies, for example: useEffect(() => { dispatch(getJobs()); }, [dispatch]); useEffect(() => { if (!isAuthorized) navigateTo('/'); }, [isAuthorized, navigateTo]);
| if (!isAuthorized) { | |
| navigateTo("/"); | |
| } | |
| }, []); | |
| if (!isAuthorized) { | |
| navigateTo("/"); | |
| } | |
| }, [dispatch]); | |
| }, [dispatch]); | |
| useEffect(() => { | |
| if (!isAuthorized) { | |
| navigateTo("/"); | |
| } | |
| }, [isAuthorized, navigateTo]); |
| "install": "^0.13.0", | ||
| "npm": "^11.4.2", |
Copilot
AI
Oct 7, 2025
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.
install and npm should not be application dependencies; they add unnecessary supply-chain risk and bloat. Remove these entries from dependencies.
| "install": "^0.13.0", | |
| "npm": "^11.4.2", |
| "start": "nodemon server.js", | ||
| "dev": "nodemon server.js" |
Copilot
AI
Oct 7, 2025
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.
Scripts use nodemon but it's not declared in dependencies/devDependencies in this package. Add nodemon as a devDependency (e.g., npm i -D nodemon) or revert scripts to node server.js.
| export const getJobs = () => async (dispatch) => { | ||
| try { | ||
| dispatch({ type: GET_JOBS_REQUEST }); | ||
| const res = await axios.get("http://localhost:4000/api/v1/job/getall"); |
Copilot
AI
Oct 7, 2025
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.
Avoid hard-coding the API URL; use an environment variable (e.g., import.meta.env.VITE_API_BASE_URL) or a centralized axios instance so builds can target different environments.
| const res = await axios.get("http://localhost:4000/api/v1/job/getall"); | |
| const res = await axios.get(`${import.meta.env.VITE_API_BASE_URL}/api/v1/job/getall`); |
| success: false, | ||
| }; | ||
|
|
||
| const jobsReducer = (state = initialState, action) => { | ||
| switch (action.type) { | ||
| case GET_JOBS_REQUEST: | ||
| return { ...state, loading: true, error: false, success: false }; | ||
| case GET_JOBS_SUCCESS: | ||
| return { ...state, loading: false, success: true, jobs: action.payload }; | ||
| case GET_JOBS_FAILURE: | ||
| return { ...state, loading: false, error: action.payload }; |
Copilot
AI
Oct 7, 2025
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.
initialState.error is a boolean but here it's replaced with a string (the error message), creating a type inconsistency. Consider keeping error as a boolean and adding errorMessage for details: return { ...state, loading: false, error: true, errorMessage: action.payload };
| success: false, | |
| }; | |
| const jobsReducer = (state = initialState, action) => { | |
| switch (action.type) { | |
| case GET_JOBS_REQUEST: | |
| return { ...state, loading: true, error: false, success: false }; | |
| case GET_JOBS_SUCCESS: | |
| return { ...state, loading: false, success: true, jobs: action.payload }; | |
| case GET_JOBS_FAILURE: | |
| return { ...state, loading: false, error: action.payload }; | |
| errorMessage: null, | |
| success: false, | |
| }; | |
| const jobsReducer = (state = initialState, action) => { | |
| switch (action.type) { | |
| case GET_JOBS_REQUEST: | |
| return { ...state, loading: true, error: false, errorMessage: null, success: false }; | |
| case GET_JOBS_SUCCESS: | |
| return { ...state, loading: false, success: true, jobs: action.payload, error: false, errorMessage: null }; | |
| case GET_JOBS_FAILURE: | |
| return { ...state, loading: false, error: true, errorMessage: action.payload }; |
…tail page