diff --git a/web/src/components/Changes/Changes.tsx b/web/src/components/Changes/Changes.tsx index f6bc548ba..2540cf282 100644 --- a/web/src/components/Changes/Changes.tsx +++ b/web/src/components/Changes/Changes.tsx @@ -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 { 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 = ({ targetBranch, sourceBranch, readOnly, - pullRequestMetadata, emptyTitle, emptyMessage, + pullRequestMetadata, className }) => { const { getString } = useStrings() @@ -67,6 +73,15 @@ export const Changes: React.FC = ({ path: `/api/v1/repos/${repoMetadata?.path}/+/compare/${targetBranch}...${sourceBranch}`, lazy: !targetBranch || !sourceBranch }) + const { + data: activities, + loading: loadingActivities, + error: errorActivities + // refetch: refetchActivities + } = useGet({ + 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 = ({ 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 ( - - {error && } + + {error && } {!loading && !error && (diffs?.length ? ( @@ -158,18 +192,24 @@ export const Changes: React.FC = ({ - + {/* TODO: lineBreaks is broken in line-by-line view, enable it for side-by-side only now */} + {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 ))} diff --git a/web/src/components/DiffViewer/DiffViewer.tsx b/web/src/components/DiffViewer/DiffViewer.tsx index 6cae6dc34..d6515548d 100644 --- a/web/src/components/DiffViewer/DiffViewer.tsx +++ b/web/src/components/DiffViewer/DiffViewer.tsx @@ -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 { 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 = ({ diff, index, viewStyle, stickyTopPosition = 0, readOnly }) => { +export const DiffViewer: React.FC = ({ + 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 = ({ diff, index, viewStyle, const { ref: inViewRef, inView } = useInView({ rootMargin: '100px 0px' }) const containerRef = useRef(null) const { currentUser } = useAppContext() - const executeDeleteComentConfirmation = useConfirmAction({ - title: getString('delete'), - intent: Intent.DANGER, - message: {getString('deleteCommentConfirm')}, - action: async ({ commentEntry, onSuccess = noop }) => { - // TODO: Delete comment - onSuccess('Delete ', commentEntry) - } - }) - - const [comments, setComments] = useState( - !index - ? [ - { - left: false, - right: true, - height: 0, - lineNumber: 11, - commentItems: [ - `Logs will looks similar to\n\nimage\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(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[]>(activitiesToDiffCommentItems(diff)) + const commentsRef = useRef[]>(comments) const setContainerRef = useCallback( node => { containerRef.current = node @@ -282,11 +273,10 @@ export const DiffViewer: React.FC = ({ diff, index, viewStyle, { 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 = ({ 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 | undefined = undefined + const id = (commentItem as CommentItem)?.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 = ({ 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 = ({ diff, index, viewStyle, - {diff.isDeleted ? diff.oldName : diff.isRename ? `${diff.oldName} -> ${diff.newName}` : diff.newName} + {diff.fileTitle}