Skip to content

Pm 4393 ai #1748

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.

Sign up for GitHub

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

Open
kkartunov wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Pm 4393 ai #1748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3953e78
Fix 4393
kkartunov Mar 19, 2026
f7e11da
Merge branch 'develop' into PM-4393-AI
kkartunov Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions src/containers/Challenges/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
setActiveProject,
resetSidebarActiveParams
} from '../../actions/sidebar'
import { checkAdmin, checkIsUserInvitedToProject } from '../../util/tc'
import { checkAdmin, checkCopilot, checkIsProjectMember, checkIsUserInvitedToProject } from '../../util/tc'
import { withRouter } from 'react-router-dom'
import { getActiveProject } from './helper'

Expand Down Expand Up @@ -126,8 +126,10 @@ class Challenges extends Component {
filterProjectOption,
projects,
activeProjectId,
projectId,
status,
projectDetail: reduxProjectInfo,
hasProjectAccess,
loadChallengesByPage,
Comment on lines 128 to 133
page,
perPage,
Comment on lines +129 to 135
Copy link

@devin-ai-integration devin-ai-integration bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down Expand Up @@ -158,13 +160,26 @@ class Challenges extends Component {
activeProjectId
)

// Check if user has access to this specific project
const isProjectRoute = !!projectId && !dashboard && !selfService
const isProjectDetailLoaded = reduxProjectInfo && `${reduxProjectInfo.id}` === `${projectId}`
const isAdmin = checkAdmin(auth.token)
const isCopilot = checkCopilot(auth.token)
const isProjectMember = isProjectDetailLoaded && checkIsProjectMember(auth.token, reduxProjectInfo)
Copy link

@github-actions github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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
Copy link

@github-actions github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 readability]
The accessDeniedMessage logic is embedded directly in the return statement. Consider extracting this logic into a separate function for better readability and maintainability.

Comment on lines +171 to +172
? "You don't have access to this project. Please contact support@topcoder.com."
: warnMessage
Comment on lines +169 to +174
Copy link

@devin-ai-integration devin-ai-integration bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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:141 sets hasProjectAccess: false but does NOT update projectDetail, so isProjectDetailLoaded (which checks reduxProjectInfo.id === projectId) is false. The condition fails on isProjectDetailLoaded.
  • On 200 (API success): The reducer at src/reducers/projects.js:147 sets hasProjectAccess: true, which makes canAccessProject (isAdmin || isCopilot || isProjectMember || hasProjectAccess) always true. 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


return (
<Fragment>
{(dashboard || activeProjectId !== -1 || selfService) && (
<ChallengesComponent
activeProject={activeProject}
fetchNextProjects={fetchNextProjects}
warnMessage={warnMessage}
warnMessage={accessDeniedMessage}
setActiveProject={setActiveProject}
dashboard={dashboard}
challenges={challenges}
Expand Down Expand Up @@ -213,6 +228,7 @@ Challenges.propTypes = {
menu: PropTypes.string,
challenges: PropTypes.arrayOf(PropTypes.object),
projectDetail: PropTypes.object,
hasProjectAccess: PropTypes.bool,
isLoading: PropTypes.bool,
loadChallengesByPage: PropTypes.func,
loadProject: PropTypes.func.isRequired,
Expand Down Expand Up @@ -259,6 +275,7 @@ const mapStateToProps = ({ challenges, sidebar, projects, auth }) => ({
activeProjectId: sidebar.activeProjectId,
projects: sidebar.projects,
projectDetail: projects.projectDetail,
hasProjectAccess: projects.hasProjectAccess,
isBillingAccountExpired: projects.isBillingAccountExpired,
billingStartDate: projects.billingStartDate,
billingEndDate: projects.billingEndDate,
Expand Down
33 changes: 25 additions & 8 deletions src/containers/ProjectEntry/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { checkIsUserInvitedToProject } from '../../util/tc'
* the invitation modal before challenge-specific requests run.
*/
const ProjectEntry = ({
hasProjectAccess,
history,
isProjectLoading,
loadOnlyProjectInfo,
Expand All @@ -24,6 +25,7 @@ const ProjectEntry = ({
}) => {
const projectId = _.get(match, 'params.projectId')
const [resolvedProjectId, setResolvedProjectId] = useState(null)
const [accessDenied, setAccessDenied] = useState(false)

useEffect(() => {
let isActive = true
Expand All @@ -34,15 +36,22 @@ const ProjectEntry = ({
}

setResolvedProjectId(null)
setAccessDenied(false)
loadOnlyProjectInfo(projectId)
.then(() => {
if (isActive) {
setResolvedProjectId(projectId)
}
})
.catch(() => {
.catch((error) => {
if (isActive) {
history.replace('/projects')
const status = _.get(error, 'payload.response.status', _.get(error, 'response.status'))
Copy link

@github-actions github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

if (status === 403) {
setAccessDenied(true)
setResolvedProjectId(projectId)
} else {
history.replace('/projects')
}
}
})

Expand All @@ -52,11 +61,17 @@ const ProjectEntry = ({
}, [history, loadOnlyProjectInfo, projectId])

useEffect(() => {
if (
!resolvedProjectId ||
isProjectLoading ||
`${_.get(projectDetail, 'id', '')}` !== `${resolvedProjectId}`
) {
if (!resolvedProjectId || isProjectLoading) {
return
}

// Handle 403 access denied - redirect to challenges page which will show the error
if (accessDenied || !hasProjectAccess) {
Copy link

@github-actions github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

history.replace(`/projects/${resolvedProjectId}/challenges`)
return
}

if (`${_.get(projectDetail, 'id', '')}` !== `${resolvedProjectId}`) {
return
}

Expand All @@ -65,12 +80,13 @@ const ProjectEntry = ({
: `/projects/${resolvedProjectId}/challenges`

history.replace(destination)
}, [history, isProjectLoading, projectDetail, resolvedProjectId, token])
}, [accessDenied, hasProjectAccess, history, isProjectLoading, projectDetail, resolvedProjectId, token])

return <Loader />
}

ProjectEntry.propTypes = {
hasProjectAccess: PropTypes.bool,
history: PropTypes.shape({
replace: PropTypes.func.isRequired
}).isRequired,
Expand All @@ -86,6 +102,7 @@ ProjectEntry.propTypes = {
}

const mapStateToProps = ({ auth, projects }) => ({
hasProjectAccess: projects.hasProjectAccess,
isProjectLoading: projects.isLoading,
projectDetail: projects.projectDetail,
token: auth.token
Expand Down
Loading