github/drone
5
0
Fork 0
mirror of https://github.com/harness/drone.git synced 2025-05-08 16:50:53 +08:00
Code Issues Packages Projects Releases Wiki Activity

Implement PR code commenting using payload field (#235)

Browse Source
This commit is contained in:
Tan Nhu 2023-01-19 16:29:38 -08:00 committed by GitHub
parent 5524dfa9ba
commit 56a6dba484
6 changed files with 281 additions and 80 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
web/src/components/Changes/Changes.tsx
View File

@ -20,9 +20,15 @@ import { useEventListener } from 'hooks/useEventListener'
import { UserPreference, useUserPreference } from 'hooks/useUserPreference'
import { PipeSeparator } from 'components/PipeSeparator/PipeSeparator'
import type { DiffFileEntry } from 'utils/types'
import { DIFF2HTML_CONFIG, ViewStyle } from 'components/DiffViewer/DiffViewerUtils'
import {
DIFF2HTML_CONFIG,
PR_CODE_COMMENT_PAYLOAD_VERSION,
PullRequestCodeCommentPayload,
ViewStyle
} from 'components/DiffViewer/DiffViewerUtils'
import { NoResultCard } from 'components/NoResultCard/NoResultCard'
import type { TypesPullReq } from 'services/code'
import type { TypesPullReq, TypesPullReqActivity } from 'services/code'
import { useShowRequestError } from 'hooks/useShowRequestError'
import { LoadingSpinner } from 'components/LoadingSpinner/LoadingSpinner'
import { ChangesDropdown } from './ChangesDropdown'
import { DiffViewConfiguration } from './DiffViewConfiguration'
@ -31,15 +37,15 @@ import css from './Changes.module.scss'
const STICKY_TOP_POSITION = 64
const STICKY_HEADER_HEIGHT = 150
const diffViewerId = (collection: Unknown[]) => collection.filter(Boolean).join('::::')
const changedFileId = (collection: Unknown[]) => collection.filter(Boolean).join('::::')
interface ChangesProps extends Pick<GitInfoProps, 'repoMetadata'> {
targetBranch?: string
sourceBranch?: string
readOnly?: boolean
pullRequestMetadata?: TypesPullReq
emptyTitle: string
emptyMessage: string
pullRequestMetadata?: TypesPullReq
className?: string
}
@ -48,9 +54,9 @@ export const Changes: React.FC<ChangesProps> = ({
targetBranch,
sourceBranch,
readOnly,
pullRequestMetadata,
emptyTitle,
emptyMessage,
pullRequestMetadata,
className
}) => {
const { getString } = useStrings()
@ -67,6 +73,15 @@ export const Changes: React.FC<ChangesProps> = ({
path: `/api/v1/repos/${repoMetadata?.path}/+/compare/${targetBranch}...${sourceBranch}`,
lazy: !targetBranch || !sourceBranch
})
const {
data: activities,
loading: loadingActivities,
error: errorActivities
// refetch: refetchActivities
} = useGet<TypesPullReqActivity[]>({
path: `/api/v1/repos/${repoMetadata.path}/+/pullreq/${pullRequestMetadata?.number}/activities`,
lazy: !pullRequestMetadata?.number
})
const diffStats = useMemo(
() =>
@ -87,25 +102,44 @@ export const Changes: React.FC<ChangesProps> = ({
if (rawDiff) {
setDiffs(
Diff2Html.parse(_raw, DIFF2HTML_CONFIG).map(diff => {
const viewerId = diffViewerId([diff.oldName, diff.newName])
const containerId = `container-${viewerId}`
const contentId = `content-${viewerId}`
const fileId = changedFileId([diff.oldName, diff.newName])
const containerId = `container-${fileId}`
const contentId = `content-${fileId}`
const fileTitle = diff.isDeleted
? diff.oldName
: diff.isRename
? `${diff.oldName} -> ${diff.newName}`
: diff.newName
const fileActivities: TypesPullReqActivity[] | undefined = activities?.filter(activity => {
const payload = activity.payload as PullRequestCodeCommentPayload
return payload?.file_id === fileId && payload?.version === PR_CODE_COMMENT_PAYLOAD_VERSION
})
return { ...diff, containerId, contentId }
return {
...diff,
containerId,
contentId,
fileId,
fileTitle,
fileActivities: fileActivities || [],
activities: activities || []
}
})
)
}
}, [rawDiff])
}, [rawDiff, activities])
useEventListener(
'scroll',
useCallback(() => setSticky(window.scrollY >= STICKY_HEADER_HEIGHT), [])
)
useShowRequestError(errorActivities)
return (
<Container className={cx(css.container, className)} {...(!!loading || !!error ? { flex: true } : {})}>
<LoadingSpinner visible={loading} />
{error && <PageError message={getErrorMessage(error)} onClick={voidFn(refetch)} />}
<LoadingSpinner visible={loading || loadingActivities} />
{error && <PageError message={getErrorMessage(error || errorActivities)} onClick={voidFn(refetch)} />}
{!loading &&
!error &&
(diffs?.length ? (
@ -158,18 +192,24 @@ export const Changes: React.FC<ChangesProps> = ({
</Layout.Horizontal>
</Container>
<Layout.Vertical spacing="large" className={cx(css.main, { [css.enableDiffLineBreaks]: lineBreaks })}>
{/* TODO: lineBreaks is broken in line-by-line view, enable it for side-by-side only now */}
<Layout.Vertical
spacing="large"
className={cx(css.main, {
[css.enableDiffLineBreaks]: lineBreaks && viewStyle === ViewStyle.SIDE_BY_SIDE
})}>
{diffs?.map((diff, index) => (
// Note: `key={viewStyle + index + lineBreaks}` resets DiffView when view configuration
// is changed. Making it easier to control states inside DiffView itself, as it does not
// have to deal with any view configuration
<DiffViewer
readOnly={readOnly}
index={index}
key={viewStyle + index + lineBreaks}
diff={diff}
viewStyle={viewStyle}
stickyTopPosition={STICKY_TOP_POSITION}
repoMetadata={repoMetadata}
pullRequestMetadata={pullRequestMetadata}
/>
))}
</Layout.Vertical>

183
web/src/components/DiffViewer/DiffViewer.tsx
View File

@ -1,4 +1,5 @@
import React, { useCallback, useEffect, useRef, useState } from 'react'
import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'
import { useMutate } from 'restful-react'
import ReactDOM from 'react-dom'
import { useInView } from 'react-intersection-observer'
import {
@ -10,35 +11,43 @@ import {
Layout,
Text,
ButtonSize,
Intent
useToaster
} from '@harness/uicore'
import cx from 'classnames'
import { Diff2HtmlUI } from 'diff2html/lib-esm/ui/js/diff2html-ui'
import { noop } from 'lodash-es'
import { useStrings } from 'framework/strings'
import { CodeIcon } from 'utils/GitUtils'
import { CodeIcon, GitInfoProps } from 'utils/GitUtils'
import { useEventListener } from 'hooks/useEventListener'
import type { DiffFileEntry } from 'utils/types'
import { useConfirmAct } from 'hooks/useConfirmAction'
import { PipeSeparator } from 'components/PipeSeparator/PipeSeparator'
import { useConfirmAction } from 'hooks/useConfirmAction'
import { useAppContext } from 'AppContext'
import type { TypesPullReq, TypesPullReqActivity } from 'services/code'
import { getErrorMessage } from 'utils/Utils'
import {
activitiesToDiffCommentItems,
activityToCommentItem,
CommentType,
DIFF2HTML_CONFIG,
DiffCommentItem,
DIFF_VIEWER_HEADER_HEIGHT,
getCommentLineInfo,
getDiffHTMLSnapshot,
getRawTextInRange,
PR_CODE_COMMENT_PAYLOAD_VERSION,
PullRequestCodeCommentPayload,
renderCommentOppositePlaceHolder,
ViewStyle
} from './DiffViewerUtils'
import { CommentBox } from '../CommentBox/CommentBox'
import { CommentAction, CommentBox, CommentItem } from '../CommentBox/CommentBox'
import css from './DiffViewer.module.scss'
interface DiffViewerProps {
index: number
interface DiffViewerProps extends Pick<GitInfoProps, 'repoMetadata'> {
diff: DiffFileEntry
viewStyle: ViewStyle
stickyTopPosition?: number
readOnly?: boolean
pullRequestMetadata?: TypesPullReq
}
//
@ -46,7 +55,14 @@ interface DiffViewerProps {
// Avoid React re-rendering at all cost as it might cause unresponsive UI
// when diff content is big, or when a PR has a lot of changed files.
//
export const DiffViewer: React.FC<DiffViewerProps> = ({ diff, index, viewStyle, stickyTopPosition = 0, readOnly }) => {
export const DiffViewer: React.FC<DiffViewerProps> = ({
diff,
viewStyle,
stickyTopPosition = 0,
readOnly,
repoMetadata,
pullRequestMetadata
}) => {
const { getString } = useStrings()
const [viewed, setViewed] = useState(false)
const [collapsed, setCollapsed] = useState(false)
@ -58,42 +74,17 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({ diff, index, viewStyle,
const { ref: inViewRef, inView } = useInView({ rootMargin: '100px 0px' })
const containerRef = useRef<HTMLDivElement | null>(null)
const { currentUser } = useAppContext()
const executeDeleteComentConfirmation = useConfirmAction({
title: getString('delete'),
intent: Intent.DANGER,
message: <Text>{getString('deleteCommentConfirm')}</Text>,
action: async ({ commentEntry, onSuccess = noop }) => {
// TODO: Delete comment
onSuccess('Delete ', commentEntry)
}
})
const [comments, setComments] = useState<DiffCommentItem[]>(
!index
? [
{
left: false,
right: true,
height: 0,
lineNumber: 11,
commentItems: [
`Logs will looks similar to\n\n<img width="1494" alt="image" src="https://user-images.githubusercontent.com/98799615/207994246-19ce9eb2-604f-4226-9a3c-6f4125d3b7cc.png">\n\n\ngitrpc logs using the \`ctx\` will have the following annotations:\n- \`grpc.service=rpc.ReferenceService\`\n- \`grpc.method=CreateBranch\`\n- \`grpc.peer=127.0.0.1:49364\`\n- \`grpc.request_id=cedrl6p1eqltblt13mgg\``,
`it seems we don't actually do anything with the explicit error type other than calling .Error(), which technically we could do on the original err object too? unless I'm missing something, could we then use errors.Is instead? (would avoid the extra var definitions at the top)`
//`If error is not converted then it will be detailed error: in BranchDelete: Branch doesn't exists. What we want is human readable error: Branch 'name' doesn't exists.`,
// `* GitRPC isolated errors, bcoz this will be probably separate repo in future and we dont want every where to include grpc status codes in our main app\n* Errors are explicit for repsonses based on error passing by types`,
// `> global ctx in wire will kill all routines, right? is this affect middlewares and interceptors? because requests should finish they work, right?\n\nI've changed the code now to pass the config directly instead of the systemstore and context, to avoid confusion (what we discussed yesterday - I remove systemstore itself another time).`
].map(content => ({
author: 'Tan Nhu',
created: '2022-12-21',
updated: '2022-12-21',
deleted: 0,
content
}))
}
]
: []
const { showError } = useToaster()
const confirmAct = useConfirmAct()
const path = useMemo(
() => `/api/v1/repos/${repoMetadata.path}/+/pullreq/${pullRequestMetadata?.number}/comments`,
[repoMetadata.path, pullRequestMetadata?.number]
)
const commentsRef = useRef<DiffCommentItem[]>(comments)
const { mutate: saveComment } = useMutate({ verb: 'POST', path })
const { mutate: updateComment } = useMutate({ verb: 'PATCH', path: ({ id }) => `${path}/${id}` })
const { mutate: deleteComment } = useMutate({ verb: 'DELETE', path: ({ id }) => `${path}/${id}` })
const [comments, setComments] = useState<DiffCommentItem<TypesPullReqActivity>[]>(activitiesToDiffCommentItems(diff))
const commentsRef = useRef<DiffCommentItem<TypesPullReqActivity>[]>(comments)
const setContainerRef = useCallback(
node => {
containerRef.current = node
@ -282,11 +273,10 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({ diff, index, viewStyle,
<CommentBox
commentItems={comment.commentItems}
getString={getString}
width={isSideBySide ? 'calc(100vw / 2 - 163px)' : undefined}
width={isSideBySide ? 'calc(100vw / 2 - 163px)' : undefined} // TODO: Re-calcualte for standalone version
onHeightChange={boxHeight => {
if (comment.height !== boxHeight) {
comment.height = boxHeight
// element.style.height = `${boxHeight}px`
setTimeout(() => setComments([...commentsRef.current]), 0)
}
}}
@ -301,7 +291,90 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({ diff, index, viewStyle,
setTimeout(() => setComments(commentsRef.current.filter(item => item !== comment)), 0)
}}
currentUserName={currentUser.display_name}
handleAction={async () => [true, undefined]} // TODO: Integrate with API
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: {
// This can be used to allow multiple-line selection when commenting
const lineNumberRange = [comment.lineNumber]
const payload: PullRequestCodeCommentPayload = {
type: CommentType.PR_CODE_COMMENT,
version: PR_CODE_COMMENT_PAYLOAD_VERSION,
file_id: diff.fileId,
file_title: diff.fileTitle,
language: diff.language || '',
is_on_left: comment.left,
at_line_number: comment.lineNumber,
line_number_range: lineNumberRange,
range_text_content: getRawTextInRange(diff, lineNumberRange),
diff_html_snapshot: getDiffHTMLSnapshot(rowElement)
}
await saveComment({ text: value, payload })
.then((newComment: TypesPullReqActivity) => {
updatedItem = activityToCommentItem(newComment)
})
.catch(exception => {
result = false
showError(getErrorMessage(exception), 0)
})
break
}
case CommentAction.REPLY: {
const rootComment = diff.fileActivities?.find(
activity => (activity.payload as PullRequestCodeCommentPayload).file_id === diff.fileId
)
if (rootComment) {
await saveComment({ text: value, parent_id: Number(rootComment.id as number) })
.then(newComment => {
updatedItem = activityToCommentItem(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
}
}
return [result, updatedItem]
}}
/>,
element
)
@ -318,7 +391,19 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({ diff, index, viewStyle,
// }
})
},
[comments, viewStyle, getString, currentUser, executeDeleteComentConfirmation, readOnly]
[
comments,
viewStyle,
getString,
currentUser,
readOnly,
diff,
saveComment,
showError,
updateComment,
deleteComment,
confirmAct
]
)
useEffect(function cleanUpCommentBoxRendering() {
@ -361,7 +446,7 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({ diff, index, viewStyle,
</Layout.Horizontal>
</Container>
<Text inline className={css.fname}>
{diff.isDeleted ? diff.oldName : diff.isRename ? `${diff.oldName} -> ${diff.newName}` : diff.newName}
{diff.fileTitle}
</Text>
<Button variation={ButtonVariation.ICON} icon={CodeIcon.Copy} size={ButtonSize.SMALL} />
<FlexExpander />

82
web/src/components/DiffViewer/DiffViewerUtils.tsx
View File

@ -1,22 +1,43 @@
import type * as Diff2Html from 'diff2html'
import HoganJsUtils from 'diff2html/lib/hoganjs-utils'
import type { CommentItem } from 'components/CommentBox/CommentBox'
import type { TypesPullReqActivity } from 'services/code'
import type { DiffFileEntry } from 'utils/types'
export enum ViewStyle {
SIDE_BY_SIDE = 'side-by-side',
LINE_BY_LINE = 'line-by-line'
}
export enum CommentType {
PR_CODE_COMMENT = 'pr_code_comment'
}
export const PR_CODE_COMMENT_PAYLOAD_VERSION = '0.1'
export interface PullRequestCodeCommentPayload {
type: CommentType
version: string // used to avoid rendering old payload structure
file_id: string // unique id of the changed file
file_title: string
language: string
is_on_left: boolean // comment made on the left side pane
at_line_number: number
line_number_range: number[]
range_text_content: string // raw text content where the comment is made
diff_html_snapshot: string // snapshot used to render diff in comment (PR Conversation). Could be used to send email notification too (with more work on capturing CSS styles and put them inline)
}
export const DIFF_VIEWER_HEADER_HEIGHT = 36
// const DIFF_MAX_CHANGES = 100
// const DIFF_MAX_LINE_LENGTH = 100
export interface DiffCommentItem {
export interface DiffCommentItem<T = Unknown> {
left: boolean
right: boolean
lineNumber: number
height: number
commentItems: CommentItem[]
commentItems: CommentItem<T>[]
}
export const DIFF2HTML_CONFIG = {
@ -159,3 +180,60 @@ export function renderCommentOppositePlaceHolder(annotation: DiffCommentItem, op
`
oppositeRowElement.after(placeHolderRow)
}
export const activityToCommentItem = (activity: TypesPullReqActivity): CommentItem<TypesPullReqActivity> => ({
author: activity.author?.display_name as string,
created: activity.created as number,
updated: activity.edited as number,
deleted: activity.deleted as number,
content: (activity.text || activity.payload?.Message) as string,
payload: activity
})
/**
* Take a small HTML snapshot of a diff in order to render code comment.
* @param atRow Row element where the comment is placed.
* @param maxNumberOfLines Maximum number of lines to take.
* @returns HTML content of the diff.
*/
export function getDiffHTMLSnapshot(atRow: HTMLTableRowElement, maxNumberOfLines = 3) {
const diffSnapshot = [atRow.outerHTML.trim()]
let prev = atRow.previousElementSibling
while (prev && diffSnapshot.length < maxNumberOfLines) {
diffSnapshot.unshift((prev.outerHTML || '').trim())
prev = prev.previousElementSibling
}
return diffSnapshot.join('')
}
export function getRawTextInRange(diff: DiffFileEntry, lineNumberRange: number[]) {
return (
(diff?.blocks[0]?.lines || [])
.filter(line => lineNumberRange.includes(line.newNumber as number))
.map(line => line.content)
.join('\n') || ''
)
}
export function activitiesToDiffCommentItems(diff: DiffFileEntry): DiffCommentItem<TypesPullReqActivity>[] {
return (
diff.fileActivities?.map(activity => {
const payload = activity.payload as PullRequestCodeCommentPayload
const replyComments =
diff.activities
?.filter(replyActivity => replyActivity.parent_id === activity.id)
.map(_activity => activityToCommentItem(_activity)) || []
return {
left: payload.is_on_left,
right: !payload.is_on_left,
height: 0,
lineNumber: payload.at_line_number,
commentItems: [activityToCommentItem(activity)].concat(replyComments)
}
}) || []
)
}

3
web/src/components/OptionsMenuButton/OptionsMenuButton.tsx
View File

@ -1,5 +1,6 @@
import React, { ReactElement } from 'react'
import cx from 'classnames'
import { omit } from 'lodash-es'
import { Classes, IMenuItemProps, Menu } from '@blueprintjs/core'
import { Button, ButtonProps } from '@harness/uicore'
import type { PopoverProps } from '@harness/uicore/dist/components/Popover/Popover'
@ -38,7 +39,7 @@ export const OptionsMenuButton = ({
<Menu.Item
key={(item as React.ComponentProps<typeof Menu.Item>)?.text as string}
className={cx(Classes.POPOVER_DISMISS, { [css.danger]: (item as OptionsMenuItem).isDanger })}
{...(item as IMenuItemProps & React.AnchorHTMLAttributes<HTMLAnchorElement>)}
{...omit(item as IMenuItemProps & React.AnchorHTMLAttributes<HTMLAnchorElement>, 'isDanger')}
/>
)
)}

20
web/src/pages/PullRequest/Conversation/Conversation.tsx
View File

@ -24,6 +24,7 @@ import { OptionsMenuButton } from 'components/OptionsMenuButton/OptionsMenuButto
import { MarkdownEditorWithPreview } from 'components/MarkdownEditorWithPreview/MarkdownEditorWithPreview'
import { useConfirmAct } from 'hooks/useConfirmAction'
import { getErrorMessage } from 'utils/Utils'
import { activityToCommentItem } from 'components/DiffViewer/DiffViewerUtils'
import { PullRequestTabContentWrapper } from '../PullRequestTabContentWrapper'
import { PullRequestStatusInfo } from './PullRequestStatusInfo/PullRequestStatusInfo'
import css from './Conversation.module.scss'
@ -53,7 +54,7 @@ export const Conversation: React.FC<ConversationProps> = ({
const threads: Record<number, CommentItem<TypesPullReqActivity>[]> = {}
activities?.forEach(activity => {
const thread: CommentItem<TypesPullReqActivity> = toCommentItem(activity)
const thread: CommentItem<TypesPullReqActivity> = activityToCommentItem(activity)
if (activity.parent_id) {
threads[activity.parent_id].push(thread)
@ -64,7 +65,7 @@ export const Conversation: React.FC<ConversationProps> = ({
})
newComments.forEach(newComment => {
threads[newComment.id as number] = [toCommentItem(newComment)]
threads[newComment.id as number] = [activityToCommentItem(newComment)]
})
return threads
@ -138,7 +139,7 @@ export const Conversation: React.FC<ConversationProps> = ({
case CommentAction.REPLY:
await saveComment({ text: value, parent_id: Number(threadId) })
.then(newComment => {
updatedItem = toCommentItem(newComment)
updatedItem = activityToCommentItem(newComment)
})
.catch(exception => {
result = false
@ -149,7 +150,7 @@ export const Conversation: React.FC<ConversationProps> = ({
case CommentAction.UPDATE:
await updateComment({ text: value }, { pathParams: { id } })
.then(newComment => {
updatedItem = toCommentItem(newComment)
updatedItem = activityToCommentItem(newComment)
})
.catch(exception => {
result = false
@ -177,7 +178,7 @@ export const Conversation: React.FC<ConversationProps> = ({
await saveComment({ text: value })
.then((newComment: TypesPullReqActivity) => {
updatedItem = toCommentItem(newComment)
updatedItem = activityToCommentItem(newComment)
setNewComments([...newComments, newComment])
})
.catch(exception => {
@ -320,12 +321,3 @@ const SystemBox: React.FC<SystemBoxProps> = ({ pullRequestMetadata, commentItems
console.warn('Unable to render system type activity', commentItems)
return null
}
const toCommentItem = (activity: TypesPullReqActivity) => ({
author: activity.author?.display_name as string,
created: activity.created as number,
updated: activity.edited as number,
deleted: activity.deleted as number,
content: (activity.text || activity.payload?.Message) as string,
payload: activity
})

5
web/src/utils/types.ts
View File

@ -1,6 +1,11 @@
import type { DiffFile } from 'diff2html/lib/types'
import type { TypesPullReqActivity } from 'services/code'
export interface DiffFileEntry extends DiffFile {
fileId: string
fileTitle: string
containerId: string
contentId: string
fileActivities?: TypesPullReqActivity[]
activities?: TypesPullReqActivity[]
}