Skip to content

[HOTFIX] - Fix broken review UI flow #1381

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
kkartunov merged 14 commits into from
Dec 12, 2025
Merged

[HOTFIX] - Fix broken review UI flow #1381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3f4c928
Potential fix for code scanning alert no. 71: Incomplete string escap…
kkartunov Dec 12, 2025
99c96d7
Potential fix for code scanning alert no. 69: Incomplete string escap…
kkartunov Dec 12, 2025
020c230
Initial plan
Copilot Dec 12, 2025
6bd27a9
Fix ESLint semicolon errors in metadataMatching.ts
Copilot Dec 12, 2025
01a64b8
Merge pull request #1378 from topcoder-platform/copilot/sub-pr-1376
kkartunov Dec 12, 2025
92a3962
Merge pull request #1376 from topcoder-platform/alert-autofix-71
kkartunov Dec 12, 2025
f1ffcf0
Merge pull request #1377 from topcoder-platform/alert-autofix-69
kkartunov Dec 12, 2025
3ec6382
Revert "fix(PM-2573): show only submissions with passed screening score"
hentrymartin Dec 12, 2025
730a58a
deploy revert
hentrymartin Dec 12, 2025
4b24324
fix: add manager comment button showing to submitters and normal revi…
hentrymartin Dec 12, 2025
667ea4e
fix: add manager comment button showing to submitters and normal revi…
hentrymartin Dec 12, 2025
2f91e54
Merge pull request #1380 from topcoder-platform/pm-3141_fix
kkartunov Dec 12, 2025
ed1eccb
Merge branch 'dev' into revert-1373-pm-2573
kkartunov Dec 12, 2025
22737bd
Merge pull request #1379 from topcoder-platform/revert-1373-pm-2573
kkartunov Dec 12, 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
1 change: 0 additions & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@ workflows:
branches:
only:
- dev
- pm-3141_3

- deployQa:
context: org-global
Expand Down
2 changes: 1 addition & 1 deletion src/apps/review/src/lib/components/FieldMarkdownEditor/FieldMarkdownEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ const toggleStrategy = {
},
italic: (start: any, end: any) => {
const startType = start.replace(/(\*|_)(?![\s\S]*(\*|_))/, '')
const endType = end.replace(/(\*|_)/, '')
const endType = end.replace(/(\*|_)/g, '')
return { endType, startType }
},
strikethrough: (start: any, end: any) => {
Expand Down
3 changes: 2 additions & 1 deletion ...cardViewer/ScorecardQuestion/ReviewResponse/ReviewManagerComment/ReviewManagerComment.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ interface ReviewManagerCommentProps {
const ReviewManagerComment: FC<ReviewManagerCommentProps> = props => {
const {
isManagerEdit,
canAddManagerComment,
isSavingManagerComment,
addManagerComment,
}: ScorecardViewerContextValue = useScorecardViewerContext()
Expand Down Expand Up @@ -131,7 +132,7 @@ const ReviewManagerComment: FC<ReviewManagerCommentProps> = props => {
</div>
)}

{!showCommentForm && !comment && isManagerEdit && (
{!showCommentForm && !comment && isManagerEdit && canAddManagerComment && (
Copy link

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

Choose a reason for hiding this comment

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

[⚠️ correctness]
The addition of canAddManagerComment to the condition ensures that the button to add a manager comment is only shown when it is allowed. Verify that canAddManagerComment is correctly set in all contexts where this component is used to avoid unexpected behavior.

<button
type='button'
onClick={handleShowCommentForm}
Expand Down
4 changes: 4 additions & 0 deletions src/apps/review/src/lib/components/Scorecard/ScorecardViewer/ScorecardViewer.context.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface ScorecardViewerContextProps {
children: ReactNode;
scorecard: Scorecard | ScorecardInfo
aiFeedbackItems?: AiFeedbackItem[]
canAddManagerComment?: boolean
reviewInfo?: ReviewInfo
isEdit?: boolean
isManagerEdit?: boolean
Expand Down Expand Up @@ -66,6 +67,7 @@ export interface ScorecardViewerContextProps {

export type ScorecardViewerContextValue = {
aiFeedbackItems?: AiFeedbackItem[]
canAddManagerComment?: boolean
toggledItems: {[key: string]: boolean}
toggleItem: (id: string, toggle?: boolean) => void
reviewInfo?: ReviewInfo
Expand Down Expand Up @@ -142,6 +144,7 @@ export const ScorecardViewerContextProvider: FC<ScorecardViewerContextProps> = p
addAppealResponse: props.addAppealResponse,
addManagerComment: props.addManagerComment,
aiFeedbackItems: props.aiFeedbackItems,
canAddManagerComment: props.canAddManagerComment,
doDeleteAppeal: props.doDeleteAppeal,
form: props.isEdit ? reviewFormCtx.form : undefined,
formErrors: props.isEdit ? reviewFormCtx.form.formState.errors : undefined,
Expand All @@ -161,6 +164,7 @@ export const ScorecardViewerContextProvider: FC<ScorecardViewerContextProps> = p
touchedAllFields: reviewFormCtx.touchedAllFields,
}), [
props.aiFeedbackItems,
props.canAddManagerComment,
props.reviewInfo,
props.isEdit,
props.isManagerEdit,
Expand Down
2 changes: 2 additions & 0 deletions src/apps/review/src/lib/components/Scorecard/ScorecardViewer/ScorecardViewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ interface ScorecardViewerProps {
isSavingAppeal?: boolean
isSavingAppealResponse?: boolean
isSavingManagerComment?: boolean
canAddManagerComment?: boolean
setReviewStatus?: (status: ReviewCtxStatus) => void
setActionButtons?: (buttons?: ReactNode) => void
saveReviewInfo?: (
Expand Down Expand Up @@ -335,6 +336,7 @@ const ScorecardViewer: FC<ScorecardViewerProps> = props => (
<ScorecardViewerContextProvider
scorecard={props.scorecard}
aiFeedbackItems={props.aiFeedback}
canAddManagerComment={props.canAddManagerComment}
reviewInfo={props.reviewInfo}
isEdit={props.isEdit}
isManagerEdit={props.isManagerEdit}
Expand Down
1 change: 1 addition & 0 deletions src/apps/review/src/lib/components/TableAppeals/TableAppeals.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ export const TableAppeals: FC<TableAppealsProps> = (props: TableAppealsProps) =>
? ownedMemberIds.has(submissionOwnerId)
: false
},
canRespondToAppeals: isAdmin || hasReviewerRole,
canViewScorecard: canAccessScorecards,
isAppealsTab: true,
}
Expand Down
5 changes: 3 additions & 2 deletions src/apps/review/src/lib/components/TableAppealsResponse/TableAppealsResponse.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,10 +319,11 @@ export const TableAppealsResponse: FC<TableAppealsResponseProps> = (props: Table
const scoreVisibilityConfig = useMemo<ScoreVisibilityConfig>(
() => ({
canDisplayScores: () => true,
canRespondToAppeals,
canViewScorecard: true,
isAppealsTab: false,
}),
[],
[canRespondToAppeals],
Copy link

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

Choose a reason for hiding this comment

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

[⚠️ performance]
The useMemo dependency array now includes canRespondToAppeals, which is correct for ensuring the memoized value updates when canRespondToAppeals changes. However, ensure that canRespondToAppeals is a stable value and doesn't change unnecessarily, as this could lead to unnecessary re-renders.

)

const columns = useMemo<TableColumn<SubmissionRow>[]>(() => {
Expand Down Expand Up @@ -467,7 +468,7 @@ export const TableAppealsResponse: FC<TableAppealsResponseProps> = (props: Table
>
<Link
className={styles.respondButton}
to={getReviewRoute(submission.id, reviewId)}
to={getReviewRoute(submission.id, reviewId, canRespondToAppeals)}
Copy link

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

Choose a reason for hiding this comment

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

[❗❗ correctness]
The addition of canRespondToAppeals as a parameter to getReviewRoute suggests that the route logic might change based on this flag. Ensure that this change is backward compatible and that all parts of the application using this route are updated accordingly to handle the new parameter.

>
Respond to Appeals
</Link>
Expand Down
20 changes: 5 additions & 15 deletions src/apps/review/src/lib/components/TableReview/TableReview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,21 +266,11 @@ export const TableReview: FC<TableReviewProps> = (props: TableReviewProps) => {
const minimumPassingScoreByScorecardId = useScorecardPassingScores(scorecardIds)

const aggregatedRows = useMemo<SubmissionRow[]>(() => {
const rows = aggregatedSubmissionRows
.filter(aggregated => {
const reviews = aggregated.reviews ?? []
const myReviewDetail = reviews.find(review => {
const resourceId = review.reviewInfo?.resourceId ?? review.resourceId
return resourceId ? myReviewerResourceIds.has(resourceId) : false
})

return !!myReviewDetail?.reviewId
})
.map(aggregated => ({
...(aggregated.submission ?? {}),
...aggregated.submission,
aggregated,
})) as SubmissionRow[]
const rows = aggregatedSubmissionRows.map(aggregated => ({
Copy link

@github-actions github-actions bot Dec 12, 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 myReviewDetail has been removed. This could lead to incorrect data being processed if the filtering was necessary to exclude certain submissions. Ensure that this change does not introduce any logical errors or unintended behavior.

...(aggregated.submission ?? {}),
...aggregated.submission,
aggregated,
})) as SubmissionRow[]

if (!restrictToLatest) {
return rows
Expand Down
10 changes: 8 additions & 2 deletions src/apps/review/src/lib/components/common/TableColumnRenderers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,7 @@ export function renderScoreCell(
canViewScorecard,
viewOwnScorecardTooltip = VIEW_OWN_SCORECARD_TOOLTIP,
getReviewUrl,
canRespondToAppeals,
}: ScoreVisibilityConfig = configWithDefaults

const reviewDetail = submission.aggregated?.reviews?.[reviewIndex]
Expand Down Expand Up @@ -585,7 +586,9 @@ export function renderScoreCell(
)
}

const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(submission.id, reviewId)
const reviewUrl = getReviewUrl
? getReviewUrl(reviewId)
: getReviewRoute(submission.id, reviewId, canRespondToAppeals)
Copy link

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

Choose a reason for hiding this comment

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

[❗❗ correctness]
The getReviewRoute function now takes an additional canRespondToAppeals parameter. Ensure that this function handles the new parameter correctly and that all usages of getReviewRoute are updated accordingly. This change could impact the correctness of the URL generation.


return (
<div className={styles.scoreReopenBlock}>
Expand Down Expand Up @@ -615,6 +618,7 @@ export function renderAppealsCell(
canViewScorecard,
viewOwnScorecardTooltip = VIEW_OWN_SCORECARD_TOOLTIP,
getReviewUrl,
canRespondToAppeals,
}: ScoreVisibilityConfig = configWithDefaults

const reviewDetail = submission.aggregated?.reviews?.[reviewIndex]
Expand Down Expand Up @@ -668,7 +672,9 @@ export function renderAppealsCell(
)
}

const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(submission.id, reviewId)
const reviewUrl = getReviewUrl
? getReviewUrl(reviewId)
: getReviewRoute(submission.id, reviewId, canRespondToAppeals)
Copy link

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

Choose a reason for hiding this comment

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

[❗❗ correctness]
The getReviewRoute function now takes an additional canRespondToAppeals parameter. Ensure that this function handles the new parameter correctly and that all usages of getReviewRoute are updated accordingly. This change could impact the correctness of the URL generation.


return (
<Link
Expand Down
2 changes: 2 additions & 0 deletions src/apps/review/src/lib/components/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ export interface ScoreVisibilityConfig {
isAppealsTab?: boolean
/** Optional function to build review detail URLs by ID. */
getReviewUrl?: (reviewId: string) => string
/** Indicates whether the viewer can appeal to respond. */
canRespondToAppeals?: boolean
}

export type { AggregatedReviewDetail, AggregatedSubmissionReviews }
11 changes: 8 additions & 3 deletions src/apps/review/src/lib/utils/metadataMatching.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,14 @@ export function findMetadataPhaseMatch(
return { source: 'stringExact' }
}

const escapedTarget = escapeRegexLiteral(target)
.replace(/ /g, '\\ ')
const sepInsensitive = new RegExp(`\\b${escapedTarget.replace(/\\ /g, '[-_\\s]+')}\\b`)
// Replace all sequences of space, underscore, or hyphen in the target with a placeholder
const WORDSEP_PLACEHOLDER = '__WORDSEP__'
const sepPattern = /[ \-_]+/g
const targetWithPlaceholder = target.replace(sepPattern, WORDSEP_PLACEHOLDER)
// Properly escape ALL regex metacharacters (including backslash), leaving the placeholder intact
const escapedTarget = escapeRegexLiteral(targetWithPlaceholder)
.replace(new RegExp(escapeRegexLiteral(WORDSEP_PLACEHOLDER), 'g'), '[-_\\s]+')
Copy link

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

Choose a reason for hiding this comment

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

[⚠️ correctness]
The use of [-_\\s]+ to replace the placeholder could potentially match unintended sequences if the target string contains special characters that are not properly escaped. Consider reviewing the escapeRegexLiteral function to ensure it handles all edge cases, or explicitly document the expected input constraints.

const sepInsensitive = new RegExp(`\\b${escapedTarget}\\b`)
if (sepInsensitive.test(normalizedMetadata)) {
return { source: 'stringBoundary' }
}
Expand Down
5 changes: 4 additions & 1 deletion src/apps/review/src/lib/utils/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const CHALLENGE_DETAILS_SEGMENT = /(active-challenges|past-challenges)\/([^/]+)\
export function getReviewRoute(
submissionId: string,
reviewId: string,
addRespondToAppeals?: boolean,
Copy link

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

Choose a reason for hiding this comment

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

[💡 readability]
Consider renaming addRespondToAppeals to includeRespondToAppeals for clarity, as it better describes the boolean nature of the parameter.

currentPathname?: string,
): string {
const encodedReviewId = encodeURIComponent(reviewId)
Expand All @@ -26,7 +27,9 @@ export function getReviewRoute(
? `${prefix}/${match[1]}/${match[2]}`
: `/${match[1]}/${match[2]}`

return `${basePath}/reviews/${submissionId}?reviewId=${encodedReviewId}&respondToAppeals=true`
const respondToAppeals = addRespondToAppeals ? '&respondToAppeals=true' : ''

return `${basePath}/reviews/${submissionId}?reviewId=${encodedReviewId}${respondToAppeals}`
}
}

Expand Down
6 changes: 6 additions & 0 deletions src/apps/review/src/pages/reviews/components/ReviewViewer/ReviewViewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,12 @@ const ReviewViewer: FC = () => {
isSavingAppeal={isSavingAppeal}
isSavingAppealResponse={isSavingAppealResponse}
isSavingManagerComment={isSavingManagerComment}
canAddManagerComment={
hasChallengeAdminRole
|| hasTopcoderAdminRole
|| hasChallengeManagerRole
|| hasChallengeCopilotRole
}
saveReviewInfo={saveReviewInfo}
addAppeal={addAppeal}
addAppealResponse={addAppealResponse}
Expand Down
Loading