Skip to content

Refactor repo-diff.ts #33746

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.

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
merged 2 commits into from
Feb 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
117 changes: 52 additions & 65 deletions web_src/js/features/repo-diff.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import $ from 'jquery';
import {initCompReactionSelector} from './comp/ReactionSelector.ts';
import {initRepoIssueContentHistory} from './repo-issue-content.ts';
import {initDiffFileTree} from './repo-diff-filetree.ts';
Expand All @@ -7,35 +6,28 @@ import {validateTextareaNonEmpty} from './comp/ComboMarkdownEditor.ts';
import {initViewedCheckboxListenerFor, countAndUpdateViewedFiles, initExpandAndCollapseFilesButton} from './pull-view-file.ts';
import {initImageDiff} from './imagediff.ts';
import {showErrorToast} from '../modules/toast.ts';
import {
submitEventSubmitter,
queryElemSiblings,
hideElem,
showElem,
animateOnce,
addDelegatedEventListener,
createElementFromHTML,
} from '../utils/dom.ts';
import {submitEventSubmitter, queryElemSiblings, hideElem, showElem, animateOnce, addDelegatedEventListener, createElementFromHTML, queryElems} from '../utils/dom.ts';
import {POST, GET} from '../modules/fetch.ts';
import {fomanticQuery} from '../modules/fomantic/base.ts';
import {createTippy} from '../modules/tippy.ts';
import {invertFileFolding} from './file-fold.ts';
import {parseDom} from '../utils.ts';

const {i18n} = window.config;

function initRepoDiffFileViewToggle() {
$('.file-view-toggle').on('click', function () {
for (const el of queryElemSiblings(this)) {
el.classList.remove('active');
}
this.classList.add('active');
// switch between "rendered" and "source", for image and CSV files
// FIXME: this event listener is not correctly added to "load more files"
queryElems(document, '.file-view-toggle', (btn) => btn.addEventListener('click', () => {
queryElemSiblings(btn, '.file-view-toggle', (el) => el.classList.remove('active'));
btn.classList.add('active');

const target = document.querySelector(this.getAttribute('data-toggle-selector'));
if (!target) return;
const target = document.querySelector(btn.getAttribute('data-toggle-selector'));
if (!target) throw new Error('Target element not found');

hideElem(queryElemSiblings(target));
showElem(target);
});
}));
}

function initRepoDiffConversationForm() {
Expand Down Expand Up @@ -103,22 +95,23 @@ function initRepoDiffConversationForm() {
}
});

$(document).on('click', '.resolve-conversation', async function (e) {
addDelegatedEventListener(document, 'click', '.resolve-conversation', async (el, e) => {
e.preventDefault();
const comment_id = $(this).data('comment-id');
const origin = $(this).data('origin');
const action = $(this).data('action');
const url = $(this).data('update-url');
const comment_id = el.getAttribute('data-comment-id');
const origin = el.getAttribute('data-origin');
const action = el.getAttribute('data-action');
const url = el.getAttribute('data-update-url');

try {
const response = await POST(url, {data: new URLSearchParams({origin, action, comment_id})});
const data = await response.text();

if ($(this).closest('.conversation-holder').length) {
const $conversation = $(data);
$(this).closest('.conversation-holder').replaceWith($conversation);
$conversation.find('.dropdown').dropdown();
initCompReactionSelector($conversation[0]);
const elConversationHolder = el.closest('.conversation-holder');
if (elConversationHolder) {
const elNewConversation = createElementFromHTML(data);
elConversationHolder.replaceWith(elNewConversation);
queryElems(elConversationHolder, '.ui.dropdown:not(.custom)', (el) => fomanticQuery(el).dropdown());
initCompReactionSelector(elNewConversation);
} else {
window.location.reload();
}
Expand All @@ -128,24 +121,19 @@ function initRepoDiffConversationForm() {
});
}

export function initRepoDiffConversationNav() {
function initRepoDiffConversationNav() {
// Previous/Next code review conversation
$(document).on('click', '.previous-conversation', (e) => {
const $conversation = $(e.currentTarget).closest('.comment-code-cloud');
const $conversations = $('.comment-code-cloud:not(.tw-hidden)');
const index = $conversations.index($conversation);
const previousIndex = index > 0 ? index - 1 : $conversations.length - 1;
const $previousConversation = $conversations.eq(previousIndex);
const anchor = $previousConversation.find('.comment').first()[0].getAttribute('id');
window.location.href = `#${anchor}`;
});
$(document).on('click', '.next-conversation', (e) => {
const $conversation = $(e.currentTarget).closest('.comment-code-cloud');
const $conversations = $('.comment-code-cloud:not(.tw-hidden)');
const index = $conversations.index($conversation);
const nextIndex = index < $conversations.length - 1 ? index + 1 : 0;
const $nextConversation = $conversations.eq(nextIndex);
const anchor = $nextConversation.find('.comment').first()[0].getAttribute('id');
addDelegatedEventListener(document, 'click', '.previous-conversation, .next-conversation', (el, e) => {
e.preventDefault();
const isPrevious = el.matches('.previous-conversation');
const elCurConversation = el.closest('.comment-code-cloud');
const elAllConversations = document.querySelectorAll('.comment-code-cloud:not(.tw-hidden)');
const index = Array.from(elAllConversations).indexOf(elCurConversation);
const previousIndex = index > 0 ? index - 1 : elAllConversations.length - 1;
const nextIndex = index < elAllConversations.length - 1 ? index + 1 : 0;
const navIndex = isPrevious ? previousIndex : nextIndex;
const elNavConversation = elAllConversations[navIndex];
const anchor = elNavConversation.querySelector('.comment').id;
window.location.href = `#${anchor}`;
});
}
Expand All @@ -161,7 +149,7 @@ function initDiffHeaderPopup() {

// Will be called when the show more (files) button has been pressed
function onShowMoreFiles() {
// FIXME: here the init calls are incomplete: at least it misses dropdown & initCompReactionSelector
// FIXME: here the init calls are incomplete: at least it misses dropdown & initCompReactionSelector & initRepoDiffFileViewToggle
initRepoIssueContentHistory();
initViewedCheckboxListenerFor();
countAndUpdateViewedFiles();
Expand All @@ -179,10 +167,11 @@ async function loadMoreFiles(btn: Element): Promise<boolean> {
try {
const response = await GET(url);
const resp = await response.text();
const $resp = $(resp);
const respDoc = parseDom(resp, 'text/html');
const respFileBoxes = respDoc.querySelector('#diff-file-boxes');
// the response is a full HTML page, we need to extract the relevant contents:
// * append the newly loaded file list items to the existing list
$('#diff-incomplete').replaceWith($resp.find('#diff-file-boxes').children());
document.querySelector('#diff-incomplete').replaceWith(...Array.from(respFileBoxes.children));
onShowMoreFiles();
return true;
} catch (error) {
Expand All @@ -200,31 +189,27 @@ function initRepoDiffShowMore() {
loadMoreFiles(el);
});

$(document).on('click', 'a.diff-load-button', async (e) => {
addDelegatedEventListener(document, 'click', 'a.diff-load-button', async (el, e) => {
e.preventDefault();
const $target = $(e.target);

if (e.target.classList.contains('disabled')) {
return;
}

e.target.classList.add('disabled');
if (el.classList.contains('disabled')) return;

const url = $target.data('href');
el.classList.add('disabled');
const url = el.getAttribute('data-href');

try {
const response = await GET(url);
const resp = await response.text();

if (!resp) {
return;
}
$target.parent().replaceWith($(resp).find('#diff-file-boxes .diff-file-body .file-body').children());
const respDoc = parseDom(resp, 'text/html');
const respFileBody = respDoc.querySelector('#diff-file-boxes .diff-file-body .file-body');
el.parentElement.replaceWith(...Array.from(respFileBody.children));
// FIXME: calling onShowMoreFiles is not quite right here.
// But since onShowMoreFiles mixes "init diff box" and "init diff body" together,
// so it still needs to call it to make the "ImageDiff" and something similar work.
onShowMoreFiles();
} catch (error) {
console.error('Error:', error);
} finally {
e.target.classList.remove('disabled');
el.classList.remove('disabled');
}
});
}
Expand Down Expand Up @@ -262,8 +247,10 @@ function initRepoDiffHashChangeListener() {
}

export function initRepoDiffView() {
initRepoDiffConversationForm();
if (!$('#diff-file-boxes').length) return;
initRepoDiffConversationForm(); // such form appears on the "conversation" page and "diff" page

if (!document.querySelector('#diff-file-boxes')) return;
initRepoDiffConversationNav(); // "previous" and "next" buttons only appear on "diff" page
initDiffFileTree();
initDiffCommitSelect();
initRepoDiffShowMore();
Expand Down
2 changes: 0 additions & 2 deletions web_src/js/features/repo-legacy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {initUnicodeEscapeButton} from './repo-unicode-escape.ts';
import {initRepoCloneButtons} from './repo-common.ts';
import {initCitationFileCopyContent} from './citation.ts';
import {initCompLabelEdit} from './comp/LabelEdit.ts';
import {initRepoDiffConversationNav} from './repo-diff.ts';
import {initCompReactionSelector} from './comp/ReactionSelector.ts';
import {initRepoSettings} from './repo-settings.ts';
import {initRepoPullRequestMergeForm} from './repo-issue-pr-form.ts';
Expand Down Expand Up @@ -64,7 +63,6 @@ export function initRepository() {
initRepoIssueWipToggle();
initRepoIssueComments();

initRepoDiffConversationNav();
initRepoIssueReferenceIssue();

initRepoIssueCommentDelete();
Expand Down