Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const isProjectDetailLoaded = reduxProjectInfo && `${reduxProjectInfo.id}` === `${projectId}` | ||
| const isAdmin = checkAdmin(auth.token) | ||
| const isCopilot = checkCopilot(auth.token) | ||
| const isProjectMember = isProjectDetailLoaded && checkIsProjectMember(auth.token, reduxProjectInfo) |
There was a problem hiding this comment.
[💡 performance]
The checkIsProjectMember function is called only if isProjectDetailLoaded is true. If checkIsProjectMember has side effects or is computationally expensive, consider whether this conditional logic is necessary or if it could be simplified.
| const canAccessProject = isAdmin || isCopilot || isProjectMember || hasProjectAccess | ||
|
|
||
| // Show access denied message if user cannot access this project | ||
| const accessDeniedMessage = isProjectRoute && isProjectDetailLoaded && !canAccessProject |
There was a problem hiding this comment.
[💡 readability]
The accessDeniedMessage logic is embedded directly in the return statement. Consider extracting this logic into a separate function for better readability and maintainability.
| .catch((error) => { | ||
| if (isActive) { | ||
| history.replace('/projects') | ||
| const status = _.get(error, 'payload.response.status', _.get(error, 'response.status')) |
There was a problem hiding this comment.
[maintainability]
Using _.get to access nested properties in the error object is a good practice to avoid runtime errors. However, consider logging the error or handling other potential error statuses to improve error handling and debugging.
| } | ||
|
|
||
| // Handle 403 access denied - redirect to challenges page which will show the error | ||
| if (accessDenied || !hasProjectAccess) { |
There was a problem hiding this comment.
[correctness]
The check for accessDenied and hasProjectAccess is correctly redirecting to the challenges page. However, ensure that the hasProjectAccess prop is always correctly set in the Redux state to avoid unexpected redirects.
There was a problem hiding this comment.
Pull request overview
This PR updates the project entry flow and challenges page to better handle “no access” scenarios (notably HTTP 403), aiming to route users to the challenges page and display an appropriate access-denied message.
Changes:
- Add 403-aware handling in
ProjectEntryto redirect forbidden project visits to the project challenges route. - Add access checks in
Challengesto compute whether the user can access the current project and overridewarnMessagewith an access-denied message. - Plumb
projects.hasProjectAccessinto both containers viamapStateToProps.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/containers/ProjectEntry/index.js | Adds access-denied state and redirects 403 responses to the project challenges route. |
| src/containers/Challenges/index.js | Adds project access evaluation (admin/copilot/member/flag) and conditionally displays an access-denied warning message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| activeProjectId, | ||
| projectId, | ||
| status, | ||
| projectDetail: reduxProjectInfo, | ||
| hasProjectAccess, | ||
| loadChallengesByPage, |
| // Show access denied message if user cannot access this project | ||
| const accessDeniedMessage = isProjectRoute && isProjectDetailLoaded && !canAccessProject |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| projectId, | ||
| status, | ||
| projectDetail: reduxProjectInfo, | ||
| hasProjectAccess, | ||
| loadChallengesByPage, | ||
| page, | ||
| perPage, |
There was a problem hiding this comment.
🔴 Duplicate projectId destructuring in const binding causes SyntaxError
projectId is destructured twice from this.props in the same const destructuring assignment (lines 129 and 140). In standard JavaScript, this is a SyntaxError — const bindings do not allow duplicate identifiers. The PR added projectId at line 129 but forgot to remove the original at line 140. Depending on the Babel transpilation pipeline, this either crashes at build time or silently produces confusing code.
(Refers to lines 129-140)
Prompt for agents
In src/containers/Challenges/index.js, the render() method's const destructuring declares `projectId` twice — at line 129 and at line 140. Remove the duplicate at line 140 (`projectId,`). The intent was to move `projectId` earlier in the destructuring (line 129) to group it with related variables, but the original on line 140 was not removed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const canAccessProject = isAdmin || isCopilot || isProjectMember || hasProjectAccess | ||
|
|
||
| // Show access denied message if user cannot access this project | ||
| const accessDeniedMessage = isProjectRoute && isProjectDetailLoaded && !canAccessProject | ||
| ? "You don't have access to this project. Please contact support@topcoder.com." | ||
| : warnMessage |
There was a problem hiding this comment.
🔴 Access denied message condition is logically unreachable — mutually exclusive guards
The access denied message at line 172 requires both isProjectDetailLoaded and !canAccessProject to be true, but these conditions are mutually exclusive:
- On 403 (API failure): The reducer at
src/reducers/projects.js:141setshasProjectAccess: falsebut does NOT updateprojectDetail, soisProjectDetailLoaded(which checksreduxProjectInfo.id === projectId) isfalse. The condition fails onisProjectDetailLoaded. - On 200 (API success): The reducer at
src/reducers/projects.js:147setshasProjectAccess: true, which makescanAccessProject(isAdmin || isCopilot || isProjectMember || hasProjectAccess) alwaystrue. The condition fails on!canAccessProject.
As a result, the access denied message string is dead code and will never be displayed to users who lack project access. The entire access-control feature added by this PR is non-functional.
Root cause: hasProjectAccess is both a condition guard and a fallback
Including hasProjectAccess in canAccessProject on line 169 makes it impossible for canAccessProject to be false when the project detail has loaded successfully, because LOAD_PROJECT_DETAILS_SUCCESS always sets hasProjectAccess: true. A possible fix is to remove isProjectDetailLoaded from the condition on line 172 and instead rely solely on !hasProjectAccess for the 403 case, OR remove hasProjectAccess from canAccessProject on line 169 and use it only in the outer condition.
Prompt for agents
In src/containers/Challenges/index.js, lines 163-174 implement an access-denied check that is logically unreachable. The condition on line 172 requires `isProjectDetailLoaded && !canAccessProject`, but these are mutually exclusive because `canAccessProject` includes `hasProjectAccess` (line 169), which is always `true` when the project detail has loaded (set by the reducer on LOAD_PROJECT_DETAILS_SUCCESS at src/reducers/projects.js:147).
To fix this, choose one of:
1. For the 403 case (project didn't load): Replace the condition on line 172 with `isProjectRoute && !hasProjectAccess` (remove the `isProjectDetailLoaded` requirement). This handles the case where the API returned 403 and no project detail is available.
2. For the membership check case (project loaded but user lacks role): Remove `hasProjectAccess` from `canAccessProject` on line 169 so the condition becomes `const canAccessProject = isAdmin || isCopilot || isProjectMember`. Then the `isProjectDetailLoaded && !canAccessProject` check on line 172 will correctly trigger when the project is loaded but the user lacks the required role.
Was this helpful? React with 👍 or 👎 to provide feedback.
Impelments the fix with Factory.AI