[UI] Fix PR Refreshing, Code Comment Canceling (#519)

This commit is contained in:
Johannes Batzill 2023-09-18 20:56:14 +00:00 committed by Harness
parent 1572618093
commit ec45e70770
10 changed files with 321 additions and 316 deletions

View File

@ -14,7 +14,7 @@ import * as Diff2Html from 'diff2html'
import cx from 'classnames' import cx from 'classnames'
import { useHistory } from 'react-router-dom' import { useHistory } from 'react-router-dom'
import { useGet } from 'restful-react' import { useGet } from 'restful-react'
import { noop } from 'lodash-es' import { isEqual, noop } from 'lodash-es'
import { useStrings } from 'framework/strings' import { useStrings } from 'framework/strings'
import type { GitInfoProps } from 'utils/GitUtils' import type { GitInfoProps } from 'utils/GitUtils'
import { PullRequestSection, formatNumber, getErrorMessage, voidFn } from 'utils/Utils' import { PullRequestSection, formatNumber, getErrorMessage, voidFn } from 'utils/Utils'
@ -48,7 +48,7 @@ interface ChangesProps extends Pick<GitInfoProps, 'repoMetadata'> {
pullRequestMetadata?: TypesPullReq pullRequestMetadata?: TypesPullReq
className?: string className?: string
onCommentUpdate: () => void onCommentUpdate: () => void
prHasChanged?: boolean prStatsChanged: Number
onDataReady?: (data: DiffFileEntry[]) => void onDataReady?: (data: DiffFileEntry[]) => void
defaultCommitRange?: string[] defaultCommitRange?: string[]
scrollElement: HTMLElement scrollElement: HTMLElement
@ -62,9 +62,9 @@ export const Changes: React.FC<ChangesProps> = ({
emptyTitle, emptyTitle,
emptyMessage, emptyMessage,
pullRequestMetadata, pullRequestMetadata,
onCommentUpdate,
className, className,
prHasChanged, onCommentUpdate,
prStatsChanged,
onDataReady, onDataReady,
defaultCommitRange, defaultCommitRange,
scrollElement scrollElement
@ -197,24 +197,31 @@ export const Changes: React.FC<ChangesProps> = ({
// happens after some comments are authored. // happens after some comments are authored.
useEffect( useEffect(
function setActivitiesIfNotSet() { function setActivitiesIfNotSet() {
if (prActivities) { if (!prActivities || isEqual(activities, prActivities)) {
setActivities(prActivities) return
} }
setActivities(prActivities)
}, },
[prActivities] [prActivities]
) )
useEffect(() => { useEffect(() => {
if (prHasChanged) { if (readOnly) {
refetchActivities() return
} }
}, [prHasChanged, refetchActivities])
refetchActivities()
}, [prStatsChanged])
useEffect(() => { useEffect(() => {
if (prHasChanged) { if (readOnly) {
refetchFileViews() return
} }
}, [prHasChanged, refetchFileViews])
refetchFileViews()
}, [prStatsChanged])
useEffect(() => { useEffect(() => {
const _raw = rawDiff && typeof rawDiff === 'string' ? rawDiff : '' const _raw = rawDiff && typeof rawDiff === 'string' ? rawDiff : ''
@ -335,7 +342,7 @@ export const Changes: React.FC<ChangesProps> = ({
// is changed. Making it easier to control states inside DiffView itself, as it does not // is changed. Making it easier to control states inside DiffView itself, as it does not
// have to deal with any view configuration // have to deal with any view configuration
<DiffViewer <DiffViewer
readOnly={readOnly} readOnly={readOnly || (commitRange?.length || 0) > 0} // render in readonly mode in case a commit is selected
key={viewStyle + index + lineBreaks} key={viewStyle + index + lineBreaks}
diff={diff} diff={diff}
viewStyle={viewStyle} viewStyle={viewStyle}

View File

@ -79,11 +79,8 @@ const CommitRangeDropdown: React.FC<CommitRangeDropdownProps> = ({
// clicked commit is outside of current range - extend it! // clicked commit is outside of current range - extend it!
const extendedArray = getCommitRange([...current, selectedCommitSHA], allCommitsSHA) const extendedArray = getCommitRange([...current, selectedCommitSHA], allCommitsSHA)
// Are all commits selected, then return AllCommits explicitly // NOTE: this CAN contain all commits - we let it through for consistent user experience.
if (extendedArray.length === allCommits.length) { // This way, the user sees selected exactly what they clicked on (+ we don't have to handle single commit pr differently)
return []
}
return extendedArray return extendedArray
}) })
} }
@ -142,9 +139,11 @@ const CommitRangeDropdown: React.FC<CommitRangeDropdownProps> = ({
color={Color.GREY_700} color={Color.GREY_700}
font={{ variation: FontVariation.BODY2 }} font={{ variation: FontVariation.BODY2 }}
margin={{ right: 'medium' }}> margin={{ right: 'medium' }}>
{selectedCommits.length && selectedCommits.length !== allCommitsSHA.length {
? `${selectedCommits.length} ${selectedCommits.length > 1 ? getString('commits') : getString('commit')}` areAllCommitsSelected
: getString('allCommits')} ? getString('allCommits')
: `${selectedCommits.length} ${selectedCommits.length > 1 ? getString('commits') : getString('commit')}`
}
</Text> </Text>
</Popover> </Popover>
) )

View File

@ -32,7 +32,7 @@ interface CommitsViewProps extends Pick<GitInfoProps, 'repoMetadata'> {
commits: TypesCommit[] | null commits: TypesCommit[] | null
emptyTitle: string emptyTitle: string
emptyMessage: string emptyMessage: string
prHasChanged?: boolean prStatsChanged?: Number
handleRefresh?: () => void handleRefresh?: () => void
showFileHistoryIcons?: boolean showFileHistoryIcons?: boolean
resourcePath?: string resourcePath?: string
@ -46,7 +46,7 @@ export function CommitsView({
emptyTitle, emptyTitle,
emptyMessage, emptyMessage,
handleRefresh = noop, handleRefresh = noop,
prHasChanged, prStatsChanged,
showFileHistoryIcons = false, showFileHistoryIcons = false,
resourcePath = '', resourcePath = '',
setActiveTab, setActiveTab,
@ -186,7 +186,7 @@ export function CommitsView({
<Container className={css.container}> <Container className={css.container}>
<Layout.Horizontal> <Layout.Horizontal>
<FlexExpander /> <FlexExpander />
{!prHasChanged ? null : ( {!prStatsChanged ? null : (
<Button <Button
onClick={handleRefresh} onClick={handleRefresh}
iconProps={{ className: css.refreshIcon, size: 12 }} iconProps={{ className: css.refreshIcon, size: 12 }}

View File

@ -40,8 +40,9 @@ import {
DiffCommentItem, DiffCommentItem,
DIFF_VIEWER_HEADER_HEIGHT, DIFF_VIEWER_HEADER_HEIGHT,
getCommentLineInfo, getCommentLineInfo,
renderCommentOppositePlaceHolder, createCommentOppositePlaceHolder,
ViewStyle ViewStyle,
contentDOMHasData
} from './DiffViewerUtils' } from './DiffViewerUtils'
import { CommentAction, CommentBox, CommentBoxOutletPosition, CommentItem } from '../CommentBox/CommentBox' import { CommentAction, CommentBox, CommentBoxOutletPosition, CommentItem } from '../CommentBox/CommentBox'
import css from './DiffViewer.module.scss' import css from './DiffViewer.module.scss'
@ -116,7 +117,31 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
const { mutate: saveComment } = useMutate({ verb: 'POST', path: commentPath }) const { mutate: saveComment } = useMutate({ verb: 'POST', path: commentPath })
const { mutate: updateComment } = useMutate({ verb: 'PATCH', path: ({ id }) => `${commentPath}/${id}` }) const { mutate: updateComment } = useMutate({ verb: 'PATCH', path: ({ id }) => `${commentPath}/${id}` })
const { mutate: deleteComment } = useMutate({ verb: 'DELETE', path: ({ id }) => `${commentPath}/${id}` }) const { mutate: deleteComment } = useMutate({ verb: 'DELETE', path: ({ id }) => `${commentPath}/${id}` })
const [comments, setComments] = useState<DiffCommentItem<TypesPullReqActivity>[]>([])
const [comments, _setComments] = useState<DiffCommentItem<TypesPullReqActivity>[]>([])
function setComments(c: DiffCommentItem<TypesPullReqActivity>[]) {
// no changes in comments? nothing to do
// NOTE: we only react to new comments as of now, not changes on existing comments or replies, so that's good enough
if (c.length == comments.length) {
return
}
_setComments(c)
triggerCodeCommentRendering()
}
// use separate flag for monitoring comment rendering as opposed to updating comments to void spamming comment changes
const [renderComments, _setRenderComments] = useState(0)
function triggerCodeCommentRendering() {
_setRenderComments(Date.now())
}
useMemo(() => {
triggerCodeCommentRendering()
}, [
viewStyle,
inView,
commitRange
])
const [dirty, setDirty] = useState(false) const [dirty, setDirty] = useState(false)
const commentsRef = useRef<DiffCommentItem<TypesPullReqActivity>[]>(comments) const commentsRef = useRef<DiffCommentItem<TypesPullReqActivity>[]>(comments)
const setContainerRef = useCallback( const setContainerRef = useCallback(
@ -145,7 +170,9 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
if (!renderCustomContent || enforced) { if (!renderCustomContent || enforced) {
containerDOM.style.height = 'auto' containerDOM.style.height = 'auto'
diffRenderer?.draw() diffRenderer?.draw()
triggerCodeCommentRendering()
} }
contentDOM.dataset.rendered = 'true' contentDOM.dataset.rendered = 'true'
} }
}, },
@ -153,22 +180,16 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
) )
useEffect(() => { useEffect(() => {
if (commitRange) { // no activities or commit range view? no comments!
if (!diff?.fileActivities || (commitRange?.length || 0) > 0) {
setComments([]) setComments([])
return
} }
}, [commitRange]) const _comments = activitiesToDiffCommentItems(diff)
if (_comments.length > 0) {
useEffect(() => { setComments(_comments)
// For some unknown reason, comments is [] when we switch to Changes tab very quickly sometimes,
// but diff is not empty, and activitiesToDiffCommentItems(diff) is not []. So assigning
// comments = activitiesToDiffCommentItems(diff) from the useState() is not enough.
if (diff) {
const _comments = activitiesToDiffCommentItems(diff)
if (_comments.length > 0 && !comments.length) {
setComments(_comments)
}
} }
}, [diff, comments]) }, [diff?.fileActivities, diff?.fileActivities?.length, commitRange])
useEffect( useEffect(
function createDiffRenderer() { function createDiffRenderer() {
@ -215,7 +236,6 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
containerClassList.remove(css.collapsed) containerClassList.remove(css.collapsed)
const newHeight = Number(containerDOM.scrollHeight) const newHeight = Number(containerDOM.scrollHeight)
if (parseInt(containerStyle.height) != newHeight) { if (parseInt(containerStyle.height) != newHeight) {
containerStyle.height = `${newHeight}px` containerStyle.height = `${newHeight}px`
} }
@ -264,7 +284,7 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
setComments([...comments, commentItem]) setComments([...comments, commentItem])
} }
}, },
[viewStyle, comments, readOnly] [viewStyle, readOnly]
), ),
containerRef.current as HTMLDivElement containerRef.current as HTMLDivElement
) )
@ -275,229 +295,205 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
return return
} }
// early exit if there's nothing to render on
if (!contentRef.current || !contentDOMHasData(contentRef.current)) {
return
}
const isSideBySide = viewStyle === ViewStyle.SIDE_BY_SIDE const isSideBySide = viewStyle === ViewStyle.SIDE_BY_SIDE
// Update latest commentsRef to use it inside CommentBox callbacks // Update latest commentsRef to use it inside CommentBox callbacks
commentsRef.current = comments commentsRef.current = comments
comments.forEach(comment => { comments.forEach(comment => {
const lineInfo = getCommentLineInfo(contentRef.current, comment, viewStyle) const lineInfo = getCommentLineInfo(contentRef.current, comment, viewStyle)
if (lineInfo.rowElement) {
const { rowElement } = lineInfo
if (lineInfo.hasCommentsRendered) { // TODO: add support for live updating changes and replies to comment!
if (isSideBySide) { if (!lineInfo.rowElement || lineInfo.hasCommentsRendered) {
const filesDiff = rowElement?.closest('.d2h-files-diff') as HTMLElement return
const sideDiff = filesDiff?.querySelector(`div.${comment.left ? 'right' : 'left'}`) as HTMLElement
const oppositeRowPlaceHolder = sideDiff?.querySelector(
`tr[data-place-holder-for-line="${comment.lineNumber}"]`
)
const first = oppositeRowPlaceHolder?.firstElementChild as HTMLTableCellElement
const last = oppositeRowPlaceHolder?.lastElementChild as HTMLTableCellElement
if (first && last) {
first.style.height = `${comment.height}px`
last.style.height = `${comment.height}px`
}
}
} else {
// Mark row that it has comment/annotation
rowElement.dataset.annotated = 'true'
// Create a new row below it and render CommentBox inside
const commentRowElement = document.createElement('tr')
commentRowElement.dataset.annotatedLine = String(comment.lineNumber)
commentRowElement.innerHTML = `<td colspan="2"></td>`
rowElement.after(commentRowElement)
const element = commentRowElement.firstElementChild as HTMLTableCellElement
const resetCommentState = (ignoreCurrentComment = true) => {
// Clean up CommentBox rendering and reset states bound to lineInfo
ReactDOM.unmountComponentAtNode(element as HTMLDivElement)
commentRowElement.parentElement?.removeChild(commentRowElement)
lineInfo.oppositeRowElement?.parentElement?.removeChild(
lineInfo.oppositeRowElement?.nextElementSibling as Element
)
delete lineInfo.rowElement.dataset.annotated
setTimeout(
() =>
setComments(
commentsRef.current.filter(item => {
if (ignoreCurrentComment) {
return item !== comment
}
return true
})
),
0
)
}
// Note: CommentBox is rendered as an independent React component
// everything passed to it must be either values, or refs. If you
// pass callbacks or states, they won't be updated and might
// cause unexpected bugs
ReactDOM.unmountComponentAtNode(element as HTMLDivElement)
ReactDOM.render(
<AppWrapper>
<CommentBox
commentItems={comment.commentItems}
initialContent={''}
width={isSideBySide ? 'calc(100vw / 2 - 163px)' : undefined} // TODO: Re-calcualte for standalone version
onHeightChange={boxHeight => {
if (comment.height !== boxHeight) {
comment.height = boxHeight
setTimeout(() => setComments([...commentsRef.current]), 0)
}
}}
onCancel={resetCommentState}
setDirty={setDirty}
currentUserName={currentUser.display_name}
handleAction={async (action, value, commentItem) => {
let result = true
let updatedItem: CommentItem<TypesPullReqActivity> | undefined = undefined
const id = (commentItem as CommentItem<TypesPullReqActivity>)?.payload?.id
switch (action) {
case CommentAction.NEW: {
const payload: OpenapiCommentCreatePullReqRequest = {
line_start: comment.lineNumber,
line_end: comment.lineNumber,
line_start_new: !comment.left,
line_end_new: !comment.left,
path: diff.filePath,
source_commit_sha: sourceRef,
target_commit_sha: targetRef,
text: value
}
await saveComment(payload)
.then((newComment: TypesPullReqActivity) => {
updatedItem = activityToCommentItem(newComment)
diff.fileActivities?.push(newComment)
comment.commentItems.push(updatedItem)
resetCommentState(false)
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0)
})
break
}
case CommentAction.REPLY: {
await saveComment({
type: CommentType.CODE_COMMENT,
text: value,
parent_id: Number(commentItem?.payload?.id as number)
})
.then(newComment => {
updatedItem = activityToCommentItem(newComment)
diff.fileActivities?.push(newComment)
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0)
})
break
}
case CommentAction.DELETE: {
result = false
await confirmAct({
message: getString('deleteCommentConfirm'),
action: async () => {
await deleteComment({}, { pathParams: { id } })
.then(() => {
result = true
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0, getString('pr.failedToDeleteComment'))
})
}
})
break
}
case CommentAction.UPDATE: {
await updateComment({ text: value }, { pathParams: { id } })
.then(newComment => {
updatedItem = activityToCommentItem(newComment)
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0)
})
break
}
}
if (result) {
onCommentUpdate()
}
return [result, updatedItem]
}}
outlets={{
[CommentBoxOutletPosition.LEFT_OF_OPTIONS_MENU]: (
<CodeCommentStatusSelect
repoMetadata={repoMetadata}
pullRequestMetadata={pullRequestMetadata as TypesPullReq}
onCommentUpdate={onCommentUpdate}
commentItems={comment.commentItems}
/>
),
[CommentBoxOutletPosition.LEFT_OF_REPLY_PLACEHOLDER]: (
<CodeCommentStatusButton
repoMetadata={repoMetadata}
pullRequestMetadata={pullRequestMetadata as TypesPullReq}
onCommentUpdate={onCommentUpdate}
commentItems={comment.commentItems}
/>
),
[CommentBoxOutletPosition.BETWEEN_SAVE_AND_CANCEL_BUTTONS]: (props: ButtonProps) => (
<CodeCommentSecondarySaveButton
repoMetadata={repoMetadata}
pullRequestMetadata={pullRequestMetadata as TypesPullReq}
commentItems={comment.commentItems}
{...props}
/>
)
}}
autoFocusAndPositioning
/>
</AppWrapper>,
element
)
// Split view: Calculate, inject, and adjust an empty place-holder row in the opposite pane
if (isSideBySide && lineInfo.oppositeRowElement) {
renderCommentOppositePlaceHolder(comment, lineInfo.oppositeRowElement)
}
}
} }
const { rowElement } = lineInfo
// Mark row that it has comment/annotation
rowElement.dataset.annotated = 'true'
// always create placeholder (in memory)
const oppositeRowPlaceHolder = createCommentOppositePlaceHolder(comment)
// in split view, actually attach the placeholder
if (isSideBySide && lineInfo.oppositeRowElement != null) {
lineInfo.oppositeRowElement.after(oppositeRowPlaceHolder)
}
// Create a new row below it and render CommentBox inside
const commentRowElement = document.createElement('tr')
commentRowElement.dataset.annotatedLine = String(comment.lineNumber)
commentRowElement.innerHTML = `<td colspan="2"></td>`
rowElement.after(commentRowElement)
const element = commentRowElement.firstElementChild as HTMLTableCellElement
const resetCommentState = () => {
// Clean up CommentBox rendering and reset states bound to lineInfo
ReactDOM.unmountComponentAtNode(element as HTMLDivElement)
commentRowElement.parentElement?.removeChild(commentRowElement)
lineInfo.oppositeRowElement?.parentElement?.removeChild(
oppositeRowPlaceHolder as Element
)
delete lineInfo.rowElement.dataset.annotated
setComments(
commentsRef.current.filter(item => {
return item !== comment
})
)
}
// Note: CommentBox is rendered as an independent React component
// everything passed to it must be either values, or refs. If you
// pass callbacks or states, they won't be updated and might
// cause unexpected bugs
ReactDOM.unmountComponentAtNode(element as HTMLDivElement)
ReactDOM.render(
<AppWrapper>
<CommentBox
commentItems={comment.commentItems}
initialContent={''}
width={isSideBySide ? 'calc(100vw / 2 - 163px)' : undefined} // TODO: Re-calcualte for standalone version
onHeightChange={boxHeight => {
const first = oppositeRowPlaceHolder?.firstElementChild as HTMLTableCellElement
const last = oppositeRowPlaceHolder?.lastElementChild as HTMLTableCellElement
if (first && last) {
first.style.height = `${boxHeight}px`
last.style.height = `${boxHeight}px`
}
}}
onCancel={resetCommentState}
setDirty={setDirty}
currentUserName={currentUser.display_name}
handleAction={async (action, value, commentItem) => {
let result = true
let updatedItem: CommentItem<TypesPullReqActivity> | undefined = undefined
const id = (commentItem as CommentItem<TypesPullReqActivity>)?.payload?.id
switch (action) {
case CommentAction.NEW: {
const payload: OpenapiCommentCreatePullReqRequest = {
line_start: comment.lineNumber,
line_end: comment.lineNumber,
line_start_new: !comment.left,
line_end_new: !comment.left,
path: diff.filePath,
source_commit_sha: sourceRef,
target_commit_sha: targetRef,
text: value
}
await saveComment(payload)
.then((newComment: TypesPullReqActivity) => {
updatedItem = activityToCommentItem(newComment)
// remove item (to refresh all comment refrences and remove it from rendering)
resetCommentState()
// add comment to file activities (will re-create comments and render new one)
diff.fileActivities?.push(newComment)
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0)
})
break
}
case CommentAction.REPLY: {
await saveComment({
type: CommentType.CODE_COMMENT,
text: value,
parent_id: Number(commentItem?.payload?.id as number)
})
.then(newComment => {
updatedItem = activityToCommentItem(newComment)
diff.fileActivities?.push(newComment)
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0)
})
break
}
case CommentAction.DELETE: {
result = false
await confirmAct({
message: getString('deleteCommentConfirm'),
action: async () => {
await deleteComment({}, { pathParams: { id } })
.then(() => {
result = true
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0, getString('pr.failedToDeleteComment'))
})
}
})
break
}
case CommentAction.UPDATE: {
await updateComment({ text: value }, { pathParams: { id } })
.then(newComment => {
updatedItem = activityToCommentItem(newComment)
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0)
})
break
}
}
if (result) {
onCommentUpdate()
}
return [result, updatedItem]
}}
outlets={{
[CommentBoxOutletPosition.LEFT_OF_OPTIONS_MENU]: (
<CodeCommentStatusSelect
repoMetadata={repoMetadata}
pullRequestMetadata={pullRequestMetadata as TypesPullReq}
onCommentUpdate={onCommentUpdate}
commentItems={comment.commentItems}
/>
),
[CommentBoxOutletPosition.LEFT_OF_REPLY_PLACEHOLDER]: (
<CodeCommentStatusButton
repoMetadata={repoMetadata}
pullRequestMetadata={pullRequestMetadata as TypesPullReq}
onCommentUpdate={onCommentUpdate}
commentItems={comment.commentItems}
/>
),
[CommentBoxOutletPosition.BETWEEN_SAVE_AND_CANCEL_BUTTONS]: (props: ButtonProps) => (
<CodeCommentSecondarySaveButton
repoMetadata={repoMetadata}
pullRequestMetadata={pullRequestMetadata as TypesPullReq}
commentItems={comment.commentItems}
{...props}
/>
)
}}
autoFocusAndPositioning
/>
</AppWrapper>,
element
)
}) })
}, },
[ [
comments, renderComments,
viewStyle,
getString,
currentUser,
readOnly,
diff,
saveComment,
showError,
updateComment,
deleteComment,
confirmAct,
onCommentUpdate,
targetRef,
sourceRef,
pullRequestMetadata,
repoMetadata
] ]
) )

View File

@ -136,6 +136,10 @@ export const DIFF2HTML_CONFIG = {
} }
} as Readonly<Diff2Html.Diff2HtmlConfig> } as Readonly<Diff2Html.Diff2HtmlConfig>
export function contentDOMHasData(contentDOM: HTMLDivElement): boolean {
return contentDOM?.querySelector('[data]') != null
}
export function getCommentLineInfo( export function getCommentLineInfo(
contentDOM: HTMLDivElement | null, contentDOM: HTMLDivElement | null,
commentEntry: DiffCommentItem, commentEntry: DiffCommentItem,
@ -143,7 +147,7 @@ export function getCommentLineInfo(
) { ) {
const isSideBySideView = viewStyle === ViewStyle.SIDE_BY_SIDE const isSideBySideView = viewStyle === ViewStyle.SIDE_BY_SIDE
const { left, lineNumber, filePath } = commentEntry const { left, lineNumber, filePath } = commentEntry
const filePathBody = filePath ? contentDOM?.querySelector(`[data="${filePath}"`) : contentDOM const filePathBody = (filePath ? contentDOM?.querySelector(`[data="${filePath}"`) : contentDOM)
const diffBody = filePathBody?.querySelector( const diffBody = filePathBody?.querySelector(
`${isSideBySideView ? `.d2h-file-side-diff${left ? '.left' : '.right'} ` : ''}.d2h-diff-tbody` `${isSideBySideView ? `.d2h-file-side-diff${left ? '.left' : '.right'} ` : ''}.d2h-diff-tbody`
@ -178,7 +182,7 @@ export function getCommentLineInfo(
} }
} }
export function renderCommentOppositePlaceHolder(annotation: DiffCommentItem, oppositeRowElement: HTMLTableRowElement) { export function createCommentOppositePlaceHolder(annotation: DiffCommentItem): HTMLTableRowElement {
const placeHolderRow = document.createElement('tr') const placeHolderRow = document.createElement('tr')
placeHolderRow.dataset.placeHolderForLine = String(annotation.lineNumber) placeHolderRow.dataset.placeHolderForLine = String(annotation.lineNumber)
@ -191,7 +195,8 @@ export function renderCommentOppositePlaceHolder(annotation: DiffCommentItem, op
</div> </div>
</td> </td>
` `
oppositeRowElement.after(placeHolderRow)
return placeHolderRow
} }
export const activityToCommentItem = (activity: TypesPullReqActivity): CommentItem<TypesPullReqActivity> => ({ export const activityToCommentItem = (activity: TypesPullReqActivity): CommentItem<TypesPullReqActivity> => ({
@ -205,7 +210,7 @@ export const activityToCommentItem = (activity: TypesPullReqActivity): CommentIt
}) })
export function activitiesToDiffCommentItems(diff: DiffFileEntry): DiffCommentItem<TypesPullReqActivity>[] { export function activitiesToDiffCommentItems(diff: DiffFileEntry): DiffCommentItem<TypesPullReqActivity>[] {
return ( const commentThreads = (
diff.fileActivities?.map(activity => { diff.fileActivities?.map(activity => {
const replyComments = const replyComments =
diff.activities diff.activities
@ -219,8 +224,11 @@ export function activitiesToDiffCommentItems(diff: DiffFileEntry): DiffCommentIt
height: 0, height: 0,
lineNumber: (right ? activity.code_comment?.line_new : activity.code_comment?.line_old) as number, lineNumber: (right ? activity.code_comment?.line_new : activity.code_comment?.line_old) as number,
commentItems: [activityToCommentItem(activity)].concat(replyComments), commentItems: [activityToCommentItem(activity)].concat(replyComments),
filePath: diff.filePath filePath: diff.filePath,
} }
}) || [] }) || []
) )
// filter out threads where all comments are deleted
return commentThreads.filter(({commentItems: commentItems}) => commentItems.map(item => !item.deleted).reduce((a,b) => a || b), false)
} }

View File

@ -277,13 +277,14 @@ export default function Compare() {
panel: ( panel: (
<TabContentWrapper loading={loading} error={error} onRetry={noop} className={css.changesContainer}> <TabContentWrapper loading={loading} error={error} onRetry={noop} className={css.changesContainer}>
<Changes <Changes
readOnly readOnly={true}
repoMetadata={repoMetadata} repoMetadata={repoMetadata}
targetRef={targetGitRef} targetRef={targetGitRef}
sourceRef={sourceGitRef} sourceRef={sourceGitRef}
emptyTitle={getString('noChanges')} emptyTitle={getString('noChanges')}
emptyMessage={getString('noChangesCompare')} emptyMessage={getString('noChangesCompare')}
onCommentUpdate={noop} onCommentUpdate={noop}
prStatsChanged={0}
scrollElement={(standalone ? document.querySelector(`.${css.main}`)?.parentElement || window : window) as HTMLElement} scrollElement={(standalone ? document.querySelector(`.${css.main}`)?.parentElement || window : window) as HTMLElement}
/> />
</TabContentWrapper> </TabContentWrapper>

View File

@ -29,7 +29,7 @@ import css from './Conversation.module.scss'
export interface ConversationProps extends Pick<GitInfoProps, 'repoMetadata' | 'pullRequestMetadata'> { export interface ConversationProps extends Pick<GitInfoProps, 'repoMetadata' | 'pullRequestMetadata'> {
onCommentUpdate: () => void onCommentUpdate: () => void
prHasChanged?: boolean prStatsChanged: Number
showEditDescription?: boolean showEditDescription?: boolean
onCancelEditDescription: () => void onCancelEditDescription: () => void
prChecksDecisionResult?: PRChecksDecisionResult prChecksDecisionResult?: PRChecksDecisionResult
@ -39,7 +39,7 @@ export const Conversation: React.FC<ConversationProps> = ({
repoMetadata, repoMetadata,
pullRequestMetadata, pullRequestMetadata,
onCommentUpdate, onCommentUpdate,
prHasChanged, prStatsChanged,
showEditDescription, showEditDescription,
onCancelEditDescription, onCancelEditDescription,
prChecksDecisionResult prChecksDecisionResult
@ -143,10 +143,10 @@ export const Conversation: React.FC<ConversationProps> = ({
) )
useEffect(() => { useEffect(() => {
if (prHasChanged) { if (prStatsChanged) {
refetchActivities() refetchActivities()
} }
}, [prHasChanged, refetchActivities, refetchReviewers]) }, [prStatsChanged, refetchActivities])
return ( return (
<PullRequestTabContentWrapper loading={showSpinner} error={error} onRetry={refetchActivities}> <PullRequestTabContentWrapper loading={showSpinner} error={error} onRetry={refetchActivities}>
@ -176,6 +176,7 @@ export const Conversation: React.FC<ConversationProps> = ({
pullRequestMetadata={pullRequestMetadata} pullRequestMetadata={pullRequestMetadata}
onCommentUpdate={onCommentUpdate} onCommentUpdate={onCommentUpdate}
onCancelEditDescription={onCancelEditDescription} onCancelEditDescription={onCancelEditDescription}
prStatsChanged={prStatsChanged}
/> />
)} )}

View File

@ -4,7 +4,7 @@ import { FontVariation } from '@harnessio/design-system'
import { useGet, useMutate } from 'restful-react' import { useGet, useMutate } from 'restful-react'
import { Render } from 'react-jsx-match' import { Render } from 'react-jsx-match'
import { useHistory } from 'react-router-dom' import { useHistory } from 'react-router-dom'
import { compact } from 'lodash-es' import { compact, isEqual } from 'lodash-es'
import { useAppContext } from 'AppContext' import { useAppContext } from 'AppContext'
import { useGetRepositoryMetadata } from 'hooks/useGetRepositoryMetadata' import { useGetRepositoryMetadata } from 'hooks/useGetRepositoryMetadata'
import { useStrings } from 'framework/strings' import { useStrings } from 'framework/strings'
@ -58,21 +58,20 @@ export default function PullRequest() {
const showSpinner = useMemo(() => { const showSpinner = useMemo(() => {
return loading || (prLoading && !prData) return loading || (prLoading && !prData)
}, [loading, prLoading, prData]) }, [loading, prLoading, prData])
const [stats, setStats] = useState<TypesPullReqStats>()
const [showEditDescription, setShowEditDescription] = useState(false) const [showEditDescription, setShowEditDescription] = useState(false)
const prHasChanged = useMemo(() => {
if (stats && prData?.stats) { const [stats, setStats] = useState<TypesPullReqStats>()
if ( // simple value one can listen on to react on stats changes (boolean is NOT enough)
stats.commits !== prData.stats.commits || const [prStatsChanged, setPrStatsChanged] = useState(0)
stats.conversations !== prData.stats.conversations || useMemo(() => {
stats.files_changed !== prData.stats.files_changed if (isEqual(stats, prData?.stats)) {
) { return
window.setTimeout(() => setStats(prData.stats), 50)
return true
}
} }
return false
setStats(stats)
setPrStatsChanged(Date.now())
}, [prData?.stats, stats]) }, [prData?.stats, stats])
const onAddDescriptionClick = useCallback(() => { const onAddDescriptionClick = useCallback(() => {
setShowEditDescription(true) setShowEditDescription(true)
history.replace( history.replace(
@ -92,43 +91,36 @@ export default function PullRequest() {
path: recheckPath, path: recheckPath,
}) })
useEffect(
function setStatsIfNotSet() {
if (!stats && prData?.stats) {
setStats(prData.stats)
}
},
[prData?.stats, stats]
)
// prData holds the latest good PR data to make sure page is not broken // prData holds the latest good PR data to make sure page is not broken
// when polling fails // when polling fails
useEffect( useEffect(
function setPrDataIfNotSet() { function setPrDataIfNotSet() {
if (pullRequestData) { if (!pullRequestData || (prData && isEqual(prData, pullRequestData))) {
// recheck pr (merge-check, ...) in case it's unavailable return
// Approximation of identifying target branch update:
// 1. branch got updated before page was loaded (status is unchecked and prData is empty)
// NOTE: This doesn't guarantee the status is UNCHECKED due to target branch update and can cause duplicate
// PR merge checks being run on PR creation or source branch update.
// 2. branch got updated while we are on the page (same source_sha but status changed to UNCHECKED)
// NOTE: This doesn't cover the case in which the status changed back to UNCHECKED before the PR is refetched.
// In that case, the user will have to re-open the PR - better than us spamming the backend with rechecks.
// This is a TEMPORARY SOLUTION and will most likely change in the future (more so on backend side)
if (pullRequestData.state == 'open' &&
pullRequestData.merge_check_status == MergeCheckStatus.UNCHECKED &&
(
// case 1:
!prData ||
// case 2:
(prData?.merge_check_status != MergeCheckStatus.UNCHECKED && prData?.source_sha == pullRequestData.source_sha)
) && !loadingRecheckPR) {
// best effort attempt to recheck PR - fail silently
recheckPR({})
}
setPrData(pullRequestData)
} }
// recheck pr (merge-check, ...) in case it's unavailable
// Approximation of identifying target branch update:
// 1. branch got updated before page was loaded (status is unchecked and prData is empty)
// NOTE: This doesn't guarantee the status is UNCHECKED due to target branch update and can cause duplicate
// PR merge checks being run on PR creation or source branch update.
// 2. branch got updated while we are on the page (same source_sha but status changed to UNCHECKED)
// NOTE: This doesn't cover the case in which the status changed back to UNCHECKED before the PR is refetched.
// In that case, the user will have to re-open the PR - better than us spamming the backend with rechecks.
// This is a TEMPORARY SOLUTION and will most likely change in the future (more so on backend side)
if (pullRequestData.state == 'open' &&
pullRequestData.merge_check_status == MergeCheckStatus.UNCHECKED &&
(
// case 1:
!prData ||
// case 2:
(prData?.merge_check_status != MergeCheckStatus.UNCHECKED && prData?.source_sha == pullRequestData.source_sha)
) && !loadingRecheckPR) {
// best effort attempt to recheck PR - fail silently
recheckPR({})
}
setPrData(pullRequestData)
}, },
[pullRequestData] [pullRequestData]
) )
@ -217,7 +209,7 @@ export default function PullRequest() {
setShowEditDescription(false) setShowEditDescription(false)
refetchPullRequest() refetchPullRequest()
}} }}
prHasChanged={prHasChanged} prStatsChanged={prStatsChanged}
showEditDescription={showEditDescription} showEditDescription={showEditDescription}
onCancelEditDescription={() => setShowEditDescription(false)} onCancelEditDescription={() => setShowEditDescription(false)}
/> />
@ -237,7 +229,7 @@ export default function PullRequest() {
<PullRequestCommits <PullRequestCommits
repoMetadata={repoMetadata as TypesRepository} repoMetadata={repoMetadata as TypesRepository}
pullRequestMetadata={prData as TypesPullReq} pullRequestMetadata={prData as TypesPullReq}
prHasChanged={prHasChanged} prStatsChanged={prStatsChanged}
handleRefresh={voidFn(refetchPullRequest)} handleRefresh={voidFn(refetchPullRequest)}
/> />
) )
@ -263,7 +255,7 @@ export default function PullRequest() {
emptyTitle={getString('noChanges')} emptyTitle={getString('noChanges')}
emptyMessage={getString('noChangesPR')} emptyMessage={getString('noChangesPR')}
onCommentUpdate={voidFn(refetchPullRequest)} onCommentUpdate={voidFn(refetchPullRequest)}
prHasChanged={prHasChanged} prStatsChanged={prStatsChanged}
scrollElement={(standalone ? document.querySelector(`.${css.main}`)?.parentElement || window : window) as HTMLElement} scrollElement={(standalone ? document.querySelector(`.${css.main}`)?.parentElement || window : window) as HTMLElement}
/> />
</Container> </Container>

View File

@ -10,14 +10,14 @@ import { CommitsView } from 'components/CommitsView/CommitsView'
import { PullRequestTabContentWrapper } from '../PullRequestTabContentWrapper' import { PullRequestTabContentWrapper } from '../PullRequestTabContentWrapper'
interface CommitProps extends Pick<GitInfoProps, 'repoMetadata' | 'pullRequestMetadata'> { interface CommitProps extends Pick<GitInfoProps, 'repoMetadata' | 'pullRequestMetadata'> {
prHasChanged: boolean prStatsChanged: Number
handleRefresh: () => void handleRefresh: () => void
} }
export const PullRequestCommits: React.FC<CommitProps> = ({ export const PullRequestCommits: React.FC<CommitProps> = ({
repoMetadata, repoMetadata,
pullRequestMetadata, pullRequestMetadata,
prHasChanged, prStatsChanged,
handleRefresh handleRefresh
}) => { }) => {
const limit = LIST_FETCHING_LIMIT const limit = LIST_FETCHING_LIMIT
@ -43,7 +43,7 @@ export const PullRequestCommits: React.FC<CommitProps> = ({
repoMetadata={repoMetadata} repoMetadata={repoMetadata}
emptyTitle={getString('noCommits')} emptyTitle={getString('noCommits')}
emptyMessage={getString('noCommitsPR')} emptyMessage={getString('noCommitsPR')}
prHasChanged={prHasChanged} prStatsChanged={prStatsChanged}
handleRefresh={voidFn(handleRefresh)} handleRefresh={voidFn(handleRefresh)}
pullRequestMetadata={pullRequestMetadata} pullRequestMetadata={pullRequestMetadata}
/> />

View File

@ -36,13 +36,14 @@ export default function RepositoryCommits() {
return ( return (
<Container className={css.changesContainer}> <Container className={css.changesContainer}>
<Changes <Changes
readOnly readOnly={true}
repoMetadata={repoMetadata} repoMetadata={repoMetadata}
targetRef={`${commitRef}~1`} targetRef={`${commitRef}~1`}
sourceRef={commitRef} sourceRef={commitRef}
emptyTitle={getString('noChanges')} emptyTitle={getString('noChanges')}
emptyMessage={getString('noChangesCompare')} emptyMessage={getString('noChangesCompare')}
onCommentUpdate={noop} onCommentUpdate={noop}
prStatsChanged={0}
scrollElement={(standalone ? document.querySelector(`.${css.main}`)?.parentElement || window : window) as HTMLElement} scrollElement={(standalone ? document.querySelector(`.${css.main}`)?.parentElement || window : window) as HTMLElement}
/> />
</Container> </Container>