From b51eea491439bbbd55adca074e2584b2606812f6 Mon Sep 17 00:00:00 2001 From: Avikalp Kumar Gupta Date: Sat, 18 Nov 2023 12:35:24 +0530 Subject: [PATCH 1/5] refactor: re-wrote the github hunk highlighting with better variable names and more readable structure --- scripts/highlighting.js | 135 +++++++++++++++++++++------------------- 1 file changed, 70 insertions(+), 65 deletions(-) diff --git a/scripts/highlighting.js b/scripts/highlighting.js index 6474b63..df7ff14 100644 --- a/scripts/highlighting.js +++ b/scripts/highlighting.js @@ -184,79 +184,84 @@ async function FilesInPrBitbucket(response) { */ const githubHunkHighlight = async (apiResponses) => { // TODO: optimization needed to not highlight next deleted line space if not present in response. - const getFileName = Array.from(document.querySelectorAll('div[data-tagsearch-path]')); - getFileName.forEach(async (item) => { - let fileContent = item.getAttribute('data-tagsearch-path'); - if (fileContent) { + const allFileDiffViews = Array.from(document.querySelectorAll('div[data-tagsearch-path]')); + allFileDiffViews.forEach(async (fileDiffView) => { + let filepathFromHTML = fileDiffView.getAttribute('data-tagsearch-path'); + if (!filepathFromHTML) { + console.error('[vibinex] File address not found in HTML'); + return; + } - const matchEncrypted = await sha256(fileContent); - const foundFiles = apiResponses["hunkinfo"].filter(item => item.filepath === matchEncrypted); + const encryptedFilepathFromHTML = await sha256(filepathFromHTML); + const relevantHunksInThisFile = apiResponses["hunkinfo"].filter(hunkInfo => hunkInfo.filepath === encryptedFilepathFromHTML); - if (foundFiles.length > 0) { - // checking for diff view either unified or split - // TODO: We can identify the view once for all files at once instead of doing it for each file separately - const deletedLines = document.querySelectorAll('input[value]'); - let diffView = false; - deletedLines.forEach((item) => { - const getValue = item.getAttribute('value'); - const getName = item.getAttribute('checked'); + if (relevantHunksInThisFile.length <= 0) { + console.debug(`[vibinex] No relevant hunks in file: ${filepathFromHTML}`); + return; + } + // checking for diff view either unified or split + // TODO: We can identify the view once for all files at once instead of doing it for each file separately + const allInputTagsWithValueAttr = document.querySelectorAll('input[value]'); + let isUnifiedDiffView = false; + allInputTagsWithValueAttr.forEach((item) => { + const valueAttrOfInputTag = item.getAttribute('value'); + const checkedAttrOfInputTag = item.getAttribute('checked'); + + if (valueAttrOfInputTag === 'unified' || valueAttrOfInputTag === 'split') { + if (checkedAttrOfInputTag === 'checked' && valueAttrOfInputTag === 'unified') { + isUnifiedDiffView = true; // for unified view + } + } + }); - if (getValue == 'unified' || getValue == 'split') { - if (getName == 'checked' && getValue == 'unified') { - diffView = true; // for unified view - } + const allRowsInFileDiff = Array.from(fileDiffView.getElementsByTagName('tr')); + if (isUnifiedDiffView) { // for unified view + const isRelevantRow = (row) => { + const addCommentButtonInLine = row.querySelector('button[data-original-line]'); + if (!addCommentButtonInLine) { + return false; // this row doesn't have 'add comment' button: interface doesn't consider it part of diff + } + const lineContent = addCommentButtonInLine.getAttribute('data-original-line'); + const signature = lineContent.charAt(0); + + const cellInRowWithLineNumber = row.querySelector('td[data-line-number]'); + const lineNumber = cellInRowWithLineNumber.getAttribute('data-line-number'); + for (const hunk of relevantHunksInThisFile) { + if ((signature === '-') && (parseInt(lineNumber) >= parseInt(hunk.line_start)) && (parseInt(lineNumber) <= parseInt(hunk.line_end))) { + return true; } + } + return false; + }; + allRowsInFileDiff.forEach((rowInFileDiff) => { + if (isRelevantRow(rowInFileDiff)) { + rowInFileDiff.style.backgroundColor = GH_RELEVANT_BG_COLOR; + } + }); + } else { // for split view + allRowsInFileDiff.forEach((rowInFileDiff) => { + if (rowInFileDiff.classList.contains('blob-expanded')) { + return; // this row is not considered part of the diff + } + const leftSideContentCell = rowInFileDiff.querySelector("td[data-split-side='left']"); + const addCommentButtonWithLineNo = leftSideContentCell.querySelector('button[data-line]'); + if (!addCommentButtonWithLineNo) { + console.error('[vibinex] Could not detect line number in diff row'); + return; // this cell doesn't have 'add comment' button + } + const lineNumber = addCommentButtonWithLineNo.getAttribute('data-line'); - }); - - const value = Array.from(item.getElementsByTagName('tr')); - - if (diffView) { - // for unified view - let flag = false; - value.forEach((item, index) => { - const deletedLines = item.querySelector('button[data-original-line]'); - if (deletedLines !== null) { - const originalLine = deletedLines.getAttribute('data-original-line'); - const signature = originalLine.charAt(0); - const tableNumber = item.querySelector('td[data-line-number]'); - const checkNumber = tableNumber.getAttribute('data-line-number'); - for (const foundFile of foundFiles) { - if ((signature == '-' || signature == '+') && parseInt(checkNumber) >= parseInt(foundFile.line_start) && parseInt(checkNumber) <= parseInt(foundFile.line_end)) { - flag = true; - } else { - flag = false; - } - - if (flag) { - item.style.backgroundColor = GH_RELEVANT_BG_COLOR; - } - } + if (leftSideContentCell && leftSideContentCell.querySelector("span[data-code-marker='-']")) { + for (const hunk of relevantHunksInThisFile) { + if (parseInt(lineNumber) >= parseInt(hunk.line_start) && parseInt(lineNumber) <= parseInt(hunk.line_end)) { + leftSideContentCell.style.backgroundColor = GH_RELEVANT_BG_COLOR; } - }); - - } else { - // for split view - value.forEach((items) => { - const secondRow = Array.from(items.getElementsByTagName('td')); - secondRow.forEach((item) => { - const buttonId = item.querySelector('button[data-line]'); - if (buttonId) { - const dataLineValue = buttonId.getAttribute('data-line'); - const tableContent = items.querySelector("td[data-split-side='left']"); - if ((tableContent.innerHTML === '') || (tableContent && tableContent.querySelector("span[data-code-marker='-']"))) { - for (const foundFile of foundFiles) { - if (parseInt(dataLineValue) >= parseInt(foundFile.line_start) && parseInt(dataLineValue) <= parseInt(foundFile.line_end)) { - items.style.backgroundColor = GH_RELEVANT_BG_COLOR; - } - } - } - } - }); - }); + } } - } + // we currently do not handle any other cases + return; + }); } }); }; From 7cccf1b7876ba8672a06628e4f0382a93c5ebe12 Mon Sep 17 00:00:00 2001 From: Avikalp Kumar Gupta Date: Sat, 18 Nov 2023 12:37:01 +0530 Subject: [PATCH 2/5] update version of chrome extension to 1.0.6 --- manifest.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest.json b/manifest.json index 1f05f28..1d1ecb5 100644 --- a/manifest.json +++ b/manifest.json @@ -1,6 +1,6 @@ { "name": "Vibinex Code Review", - "version": "1.0.5", + "version": "1.0.6", "manifest_version": 3, "description": "Personalization and context for pull requests on GitHub & Bitbucket", "key": "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsRm6EaBdHDBxVjt9o9WKeL9EDdz1X+knDAU5uoZaRsXTmWjslhJN9DhSd7/Ys4aJOSN+s+5/HnIHcKV63P4GYaUM5FhETHEWORHlwIgjcV/1h6wD6bNbvXi06gtiygE+yMrCzzD93/Z+41XrwMElYiW2U5owNpat2Yfq4p9FDX1uBJUKsRIMp6LbRQla4vAzH/HMUtHWmeuUsmPVzcq1b6uB1QmuJqIQ1GrntIHw3UBWUlqRZ5OtxI1DCP3knglvqz26WT5Pc4GBDNlcI9+3F0vhwqwHqrdyjZpIKZ7iaQzcrovOqUKuXs1J3hDtXq8WoJELIqfIisY7rhAvq6b8jQIDAQAB", From e0daf3ecef4ab432e3997a76fb09a78f8318a101 Mon Sep 17 00:00:00 2001 From: Avikalp Kumar Gupta Date: Thu, 23 Nov 2023 10:36:53 +0530 Subject: [PATCH 3/5] bug fix: [highlighting/github] Added a null check --- scripts/highlighting.js | 4 ++++ scripts/orchestrator.js | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/highlighting.js b/scripts/highlighting.js index df7ff14..9d09866 100644 --- a/scripts/highlighting.js +++ b/scripts/highlighting.js @@ -245,6 +245,10 @@ const githubHunkHighlight = async (apiResponses) => { return; // this row is not considered part of the diff } const leftSideContentCell = rowInFileDiff.querySelector("td[data-split-side='left']"); + if (!leftSideContentCell) { + console.warn(`[vibinex] Could not detect left side content cell in diff row`); + return; // this row doesn't have left side content cell + } const addCommentButtonWithLineNo = leftSideContentCell.querySelector('button[data-line]'); if (!addCommentButtonWithLineNo) { console.error('[vibinex] Could not detect line number in diff row'); diff --git a/scripts/orchestrator.js b/scripts/orchestrator.js index 117f758..c5da38a 100644 --- a/scripts/orchestrator.js +++ b/scripts/orchestrator.js @@ -81,9 +81,9 @@ const orchestrator = async (tabUrl, websiteUrl, userId) => { // for showing all tracked repo in user page else if (urlObj[3] && !urlObj[4] && searchParams.includes('tab=repositories')) { // for working on this url https://github.com/username?tab=repositories - const userName = urlObj[3] + const userName = urlObj[3]; const trackedRepos = await getTrackedRepos(userName, userId, 'github'); - updateTrackedReposInGitHub(trackedRepos, websiteUrl, 'user') + updateTrackedReposInGitHub(trackedRepos, websiteUrl, 'user'); } } From a795df09e745c36184726d703b940fd6c113fc66 Mon Sep 17 00:00:00 2001 From: Avikalp Kumar Gupta Date: Thu, 23 Nov 2023 10:57:41 +0530 Subject: [PATCH 4/5] handled 2 code smells in GitHub highlighting function --- scripts/highlighting.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/highlighting.js b/scripts/highlighting.js index 9d09866..cdb9e0e 100644 --- a/scripts/highlighting.js +++ b/scripts/highlighting.js @@ -256,7 +256,7 @@ const githubHunkHighlight = async (apiResponses) => { } const lineNumber = addCommentButtonWithLineNo.getAttribute('data-line'); - if (leftSideContentCell && leftSideContentCell.querySelector("span[data-code-marker='-']")) { + if (leftSideContentCell?.querySelector("span[data-code-marker='-']")) { for (const hunk of relevantHunksInThisFile) { if (parseInt(lineNumber) >= parseInt(hunk.line_start) && parseInt(lineNumber) <= parseInt(hunk.line_end)) { leftSideContentCell.style.backgroundColor = GH_RELEVANT_BG_COLOR; @@ -264,7 +264,6 @@ const githubHunkHighlight = async (apiResponses) => { } } // we currently do not handle any other cases - return; }); } }); From d594d58c21dda1bedb4073f25cd7075944869c8e Mon Sep 17 00:00:00 2001 From: Avikalp Kumar Gupta Date: Tue, 28 Nov 2023 11:42:39 +0530 Subject: [PATCH 5/5] 1. Unified or Split view is only detected once 2. Unified view highlighting works even for users not part of organization (no 'add comment' button) 3. Better error logging --- scripts/highlighting.js | 51 +++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/scripts/highlighting.js b/scripts/highlighting.js index cdb9e0e..f4528c0 100644 --- a/scripts/highlighting.js +++ b/scripts/highlighting.js @@ -183,12 +183,26 @@ async function FilesInPrBitbucket(response) { * @param {Array} apiResponses - Array of API responses containing hunk information. */ const githubHunkHighlight = async (apiResponses) => { + // checking for diff view either unified or split + let isUnifiedDiffView = false; + const allInputTagsWithValueAttr = document.querySelectorAll('input[value]'); // for reducing DOM queries in foreach loop. + for (const item of allInputTagsWithValueAttr) { + const valueAttrOfInputTag = item.getAttribute('value'); + const checkedAttrOfInputTag = item.getAttribute('checked'); + + if (valueAttrOfInputTag === 'unified' || valueAttrOfInputTag === 'split') { + if (checkedAttrOfInputTag === 'checked' && valueAttrOfInputTag === 'unified') { + isUnifiedDiffView = true; // for unified view + } + } + }; + // TODO: optimization needed to not highlight next deleted line space if not present in response. const allFileDiffViews = Array.from(document.querySelectorAll('div[data-tagsearch-path]')); allFileDiffViews.forEach(async (fileDiffView) => { let filepathFromHTML = fileDiffView.getAttribute('data-tagsearch-path'); if (!filepathFromHTML) { - console.error('[vibinex] File address not found in HTML'); + console.error(`[vibinex] File address not found in HTML for file: ${filepathFromHTML}`); return; } @@ -199,33 +213,26 @@ const githubHunkHighlight = async (apiResponses) => { console.debug(`[vibinex] No relevant hunks in file: ${filepathFromHTML}`); return; } - // checking for diff view either unified or split - // TODO: We can identify the view once for all files at once instead of doing it for each file separately - const allInputTagsWithValueAttr = document.querySelectorAll('input[value]'); - let isUnifiedDiffView = false; - allInputTagsWithValueAttr.forEach((item) => { - const valueAttrOfInputTag = item.getAttribute('value'); - const checkedAttrOfInputTag = item.getAttribute('checked'); - - if (valueAttrOfInputTag === 'unified' || valueAttrOfInputTag === 'split') { - if (checkedAttrOfInputTag === 'checked' && valueAttrOfInputTag === 'unified') { - isUnifiedDiffView = true; // for unified view - } - } - }); - - const allRowsInFileDiff = Array.from(fileDiffView.getElementsByTagName('tr')); + /** Select all TRs except the following: + * - TRs with the .js-skip-tagsearch are the rows which are expandable (we click on it to see more code) + * - TRs inside the THEAD[hidden] tags only contain the headings of the diff table + */ + const allRowsInFileDiff = Array.from(fileDiffView.querySelectorAll('tr:not(.js-skip-tagsearch):not(thead[hidden] :is(tr))')); if (isUnifiedDiffView) { // for unified view const isRelevantRow = (row) => { - const addCommentButtonInLine = row.querySelector('button[data-original-line]'); - if (!addCommentButtonInLine) { - return false; // this row doesn't have 'add comment' button: interface doesn't consider it part of diff + const lineContentSpan = row.querySelector('span[data-code-marker]'); + if (!lineContentSpan) { + console.warn(`[vibinex] Could not detect span with data-code-marker in diff row for file: ${filepathFromHTML}`); + return false; } - const lineContent = addCommentButtonInLine.getAttribute('data-original-line'); - const signature = lineContent.charAt(0); + const signature = lineContentSpan.getAttribute('data-code-marker'); const cellInRowWithLineNumber = row.querySelector('td[data-line-number]'); + if (!cellInRowWithLineNumber) { + console.warn(`[vibinex] Could not detect cell with data-line-number in diff row for file: ${filepathFromHTML}`); + return false; + } const lineNumber = cellInRowWithLineNumber.getAttribute('data-line-number'); for (const hunk of relevantHunksInThisFile) { if ((signature === '-') && (parseInt(lineNumber) >= parseInt(hunk.line_start)) && (parseInt(lineNumber) <= parseInt(hunk.line_end))) {