Skip to content

Pm 3255 #1388

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

Merged
jmgasper merged 3 commits into from
Dec 17, 2025
Merged

Pm 3255 #1388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac1250c
Fix for submitter list on submissions tab for submitter role to only …
jmgasper Dec 17, 2025
344cd89
Revert start.sh dev changes
jmgasper Dec 17, 2025
4fcb67f
Revert start.sh dev changes
jmgasper Dec 17, 2025
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/apps/review/src/lib/contexts/ChallengeDetailContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ import { FC, PropsWithChildren, useContext, useMemo } from 'react'
import { useParams } from 'react-router-dom'

import { convertBackendSubmissionToSubmissionInfo } from '../models'
import type { ChallengeDetailContextModel, SubmissionInfo } from '../models'
import type {
ChallengeDetailContextModel,
ReviewAppContextModel,
SubmissionInfo,
} from '../models'
import {
useFetchChallengeInfo,
useFetchChallengeInfoProps,
Expand All @@ -16,11 +20,13 @@ import {
} from '../hooks'

import { ChallengeDetailContext } from './ChallengeDetailContext'
import { ReviewAppContext } from './ReviewAppContext'

export const ChallengeDetailContextProvider: FC<PropsWithChildren> = props => {
const { challengeId = '' }: { challengeId?: string } = useParams<{
challengeId: string
}>()
const { loginUserInfo }: ReviewAppContextModel = useContext(ReviewAppContext)
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[⚠️ correctness]
The destructuring of loginUserInfo from ReviewAppContext assumes that useContext(ReviewAppContext) will always return a valid object. Consider adding a default value or null check to prevent potential runtime errors if the context is not properly initialized.

// fetch challenge info
const {
challengeInfo,
Expand All @@ -37,10 +43,21 @@ export const ChallengeDetailContextProvider: FC<PropsWithChildren> = props => {
isLoading: isLoadingChallengeResources,
resourceMemberIdMapping,
}: useFetchChallengeResourcesProps = useFetchChallengeResources(challengeId)
const submissionViewer = useMemo(
() => ({
roles: myRoles,
tokenRoles: loginUserInfo?.roles,
userId: loginUserInfo?.userId,
}),
[loginUserInfo?.roles, loginUserInfo?.userId, myRoles],
)
const {
challengeSubmissions,
isLoading: isLoadingChallengeSubmissions,
}: useFetchChallengeSubmissionsProps = useFetchChallengeSubmissions(challengeId)
}: useFetchChallengeSubmissionsProps = useFetchChallengeSubmissions(
challengeId,
submissionViewer,
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[⚠️ performance]
The submissionViewer object is passed as a dependency to useFetchChallengeSubmissions. Ensure that useFetchChallengeSubmissions is memoized or stable, as changes in submissionViewer will trigger re-fetching, which could lead to performance issues if not handled correctly.

)

const submissionInfos = useMemo<SubmissionInfo[]>(
() => challengeSubmissions.map(convertBackendSubmissionToSubmissionInfo),
Expand Down
113 changes: 111 additions & 2 deletions src/apps/review/src/lib/hooks/useFetchChallengeSubmissions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
import useSWR, { SWRResponse } from 'swr'

import { handleError } from '~/libs/shared'
import { UserRole } from '~/libs/core'

import { BackendSubmission, BackendSubmissionStatus } from '../models'
import { fetchSubmissions } from '../services'
Expand All @@ -22,13 +23,20 @@ interface ChallengeSubmissionsMemoResult {
filteredSubmissions: BackendSubmission[]
}

export interface ChallengeSubmissionsViewer {
roles?: Array<string | undefined | null>
tokenRoles?: Array<string | undefined | null>
userId?: string | number | null
}

/**
* Fetch challenge submissions
* @param challengeId challenge id
* @returns challenge submissions
*/
export function useFetchChallengeSubmissions(
challengeId?: string,
viewer?: ChallengeSubmissionsViewer,
): useFetchChallengeSubmissionsProps {
// Use swr hooks for submissions fetching
const {
Expand All @@ -46,6 +54,92 @@ export function useFetchChallengeSubmissions(
isPaused: () => !challengeId,
})

const normalizedRoles = useMemo<string[]>(
() => (viewer?.roles ?? [])
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[⚠️ correctness]
The normalization of roles involves converting each role to a string and trimming it. Consider handling cases where roles might not be strings initially to avoid potential runtime errors.

.map(role => (role ? `${role}`.toLowerCase()
.trim() : ''))
.filter(Boolean),
[viewer?.roles],
)
const normalizedTokenRoles = useMemo<string[]>(
() => (viewer?.tokenRoles ?? [])
.map(role => (typeof role === 'string' ? role.toLowerCase()
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[⚠️ correctness]
The normalization of token roles assumes roles are strings. Ensure that all roles are validated as strings before processing to prevent unexpected behavior.

.trim() : ''))
.filter(Boolean),
[viewer?.tokenRoles],
)
const hasSubmitterRole = useMemo<boolean>(
() => normalizedRoles.some(role => role.includes('submitter')),
[normalizedRoles],
)
const hasCopilotRole = useMemo<boolean>(
() => normalizedRoles.some(role => role.includes('copilot')),
[normalizedRoles],
)
const hasReviewerRole = useMemo<boolean>(
() => normalizedRoles.some(role => role.includes('reviewer')),
[normalizedRoles],
)
const hasManagerRole = useMemo<boolean>(
() => normalizedRoles.some(role => role.includes('manager')),
[normalizedRoles],
)
const hasScreenerRole = useMemo<boolean>(
() => normalizedRoles.some(role => role.includes('screener')),
[normalizedRoles],
)
const hasApproverRole = useMemo<boolean>(
() => normalizedRoles.some(role => role.includes('approver')),
[normalizedRoles],
)
const isProjectManager = useMemo<boolean>(
() => normalizedTokenRoles.some(
role => role === UserRole.projectManager.toLowerCase(),
)
|| normalizedRoles.some(role => role.includes('project manager')),
[normalizedRoles, normalizedTokenRoles],
)
const isAdmin = useMemo<boolean>(
() => normalizedTokenRoles.some(
role => role === UserRole.administrator.toLowerCase(),
)
|| normalizedRoles.some(role => role.includes('admin')),
[normalizedRoles, normalizedTokenRoles],
)
const canViewAllSubmissions = useMemo<boolean>(
() => (viewer ? (
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[⚠️ security]
The logic for canViewAllSubmissions assumes that if viewer is not provided, all submissions can be viewed. Verify that this behavior aligns with the intended access control policy.

isAdmin
|| hasCopilotRole
|| hasReviewerRole
|| hasManagerRole
|| hasScreenerRole
|| hasApproverRole
|| isProjectManager
) : true),
[
viewer,
isAdmin,
hasCopilotRole,
hasReviewerRole,
hasManagerRole,
hasScreenerRole,
hasApproverRole,
isProjectManager,
],
)
const viewerMemberId = useMemo<string | undefined>(
() => {
const raw = viewer?.userId
if (raw === undefined || raw === null) {
return undefined
}

const normalized = `${raw}`.trim()
return normalized.length ? normalized : undefined
},
[viewer?.userId],
)

// Show backend error when fetching data fail
useEffect(() => {
if (error) {
Expand All @@ -69,6 +163,10 @@ export function useFetchChallengeSubmissions(
const normalizedDeletedIds = new Set<string>()
const normalizedDeletedLegacyIds = new Set<string>()
const activeSubmissions: BackendSubmission[] = []
const shouldRestrictToCurrentMember = Boolean(
hasSubmitterRole
&& !canViewAllSubmissions,
)

const normalizeStatus = (status: unknown): string => {
if (typeof status === 'string') {
Expand Down Expand Up @@ -105,12 +203,23 @@ export function useFetchChallengeSubmissions(
activeSubmissions.push(submission)
})

const visibleSubmissions = shouldRestrictToCurrentMember
? activeSubmissions.filter(submission => (viewerMemberId
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[⚠️ correctness]
The filtering logic for visibleSubmissions uses string comparison for memberId. Ensure that memberId is consistently a string across all relevant data sources to avoid mismatches.

? `${submission?.memberId ?? ''}` === viewerMemberId
: false))
: activeSubmissions

return {
deletedLegacySubmissionIds: normalizedDeletedLegacyIds,
deletedSubmissionIds: normalizedDeletedIds,
filteredSubmissions: activeSubmissions,
filteredSubmissions: visibleSubmissions,
}
}, [challengeSubmissions])
}, [
challengeSubmissions,
canViewAllSubmissions,
hasSubmitterRole,
viewerMemberId,
])

return {
challengeSubmissions: filteredSubmissions,
Expand Down
14 changes: 13 additions & 1 deletion src/apps/review/src/lib/hooks/useFetchScreeningReview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,16 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
reviewers: challengeReviewers,
resources,
myResources,
myRoles,
}: ChallengeDetailContextModel = useContext(ChallengeDetailContext)
const submissionViewer = useMemo(
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[💡 performance]
The useMemo hook is used to memoize the submissionViewer object. Ensure that the dependencies [loginUserInfo?.roles, loginUserInfo?.userId, myRoles] are sufficient and cover all properties accessed within the memoized function. If any of these properties change frequently, consider whether memoization provides a significant performance benefit.

() => ({
roles: myRoles,
tokenRoles: loginUserInfo?.roles,
userId: loginUserInfo?.userId,
}),
[loginUserInfo?.roles, loginUserInfo?.userId, myRoles],
)

const challengeLegacy = (challengeInfo as unknown as {
legacy?: {
Expand All @@ -451,7 +460,10 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
deletedLegacySubmissionIds,
deletedSubmissionIds,
isLoading,
}: useFetchChallengeSubmissionsProps = useFetchChallengeSubmissions(challengeId)
}: useFetchChallengeSubmissionsProps = useFetchChallengeSubmissions(
Copy link

@github-actions github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

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

[⚠️ correctness]
The useFetchChallengeSubmissions function now takes submissionViewer as an additional argument. Ensure that this change is reflected in the implementation of useFetchChallengeSubmissions and that it handles the submissionViewer object correctly. Verify that this does not introduce any unintended side effects.

challengeId,
submissionViewer,
)

const visibleChallengeSubmissions = useMemo<BackendSubmission[]>(
() => challengeSubmissions,
Expand Down
5 changes: 2 additions & 3 deletions start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@

export ESLINT_NO_DEV_ERRORS=true
export HTTPS=true
export HOST=local.topcoder-dev.com
export REACT_APP_HOST_ENV=${REACT_APP_HOST_ENV:-dev}
export PORT=443

export REACT_APP_HOST_ENV=${REACT_APP_HOST_ENV:-dev}
export HOST=local.topcoder-dev.com
# if [[ ! -e ./.environments/.env.local ]]; then
# filename=.env.${REACT_APP_HOST_ENV:-dev}
# cp ./.environments/$filename ./.environments/.env.local
Expand Down
Loading