Skip to content

Use git diff-tree for DiffFileTree on diff pages #33514

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 8 commits into from
Feb 28, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion routers/web/repo/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ func Diff(ctx *context.Context) {
return
}

ctx.Data["DiffFiles"] = transformDiffTreeForUI(diffTree, nil)
ctx.PageData["DiffFiles"] = transformDiffTreeForUI(diffTree, nil)
}

statuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, commitID, db.ListOptionsAll)
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ func PrepareCompareDiff(
return false
}

ctx.Data["DiffFiles"] = transformDiffTreeForUI(diffTree, nil)
ctx.PageData["DiffFiles"] = transformDiffTreeForUI(diffTree, nil)
}

headCommit, err := ci.HeadGitRepo.GetCommit(headCommitID)
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
}
}

ctx.Data["DiffFiles"] = transformDiffTreeForUI(diffTree, filesViewedState)
ctx.PageData["DiffFiles"] = transformDiffTreeForUI(diffTree, filesViewedState)
}

ctx.Data["Diff"] = diff
Expand Down
22 changes: 0 additions & 22 deletions templates/repo/diff/box.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -57,28 +57,6 @@
<div>{{ctx.Locale.Tr "repo.pulls.showing_specified_commit_range" (ShortSha .BeforeCommitID) (ShortSha .AfterCommitID)}} - <a href="{{$.Issue.Link}}/files?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}">{{ctx.Locale.Tr "repo.pulls.show_all_commits"}}</a></div>
</div>
{{end}}
<script id="diff-data-script" type="module">
const diffDataFiles = [{{range $i, $file := .DiffFiles}}
{Name:"{{$file.Name}}",NameHash:"{{$file.NameHash}}",Status:{{$file.Status}},IsSubmodule:{{$file.IsSubmodule}},IsViewed:{{$file.IsViewed}}},{{end}}];

const diffData = {
isIncomplete: {{.Diff.IsIncomplete}},
};

// for first time loading, the diffFileInfo is a plain object
// after the Vue component is mounted, the diffFileInfo is a reactive object
let diffFileInfo = window.config.pageData.diffFileInfo || {
files:[],
fileTreeIsVisible: false,
fileListIsVisible: false,
isLoadingNewData: false,
selectedItem: '',
};
diffFileInfo = Object.assign(diffFileInfo, diffData);
diffFileInfo.files.push(...diffDataFiles);
window.config.pageData.diffFileInfo = diffFileInfo;
</script>
<div id="diff-file-list"></div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this removal, will it not flash wrong content on page load when diff tree is visible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I may be misunderstanding and this was just some data modification that has no reason to be in templates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's not related to "content flash" (and this block was marked as "module", it only executes after DOM ready).

Some old PRs used very tricky methods to make the Vue work, now it's the time to make code overall right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we should remove all such template js, except those cases that serve the purpose of avoiding flash, but those are better migrated to web components.

{{end}}
<div id="diff-container">
{{if $showFileTree}}
Expand Down
91 changes: 29 additions & 62 deletions web_src/js/features/repo-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {fomanticQuery} from '../modules/fomantic/base.ts';
import {createTippy} from '../modules/tippy.ts';
import {invertFileFolding} from './file-fold.ts';

const {pageData, i18n} = window.config;
const {i18n} = window.config;

function initRepoDiffFileViewToggle() {
$('.file-view-toggle').on('click', function () {
Expand Down Expand Up @@ -168,39 +168,35 @@ function onShowMoreFiles() {
initDiffHeaderPopup();
}

async function loadMoreFiles(url: string) {
const target = document.querySelector('a#diff-show-more-files');
if (target?.classList.contains('disabled') || pageData.diffFileInfo.isLoadingNewData) {
return;
async function loadMoreFiles(btn: Element): Promise<boolean> {
if (btn.classList.contains('disabled')) {
return false;
}

pageData.diffFileInfo.isLoadingNewData = true;
target?.classList.add('disabled');

btn.classList.add('disabled');
const url = btn.getAttribute('data-href');
try {
const response = await GET(url);
const resp = await response.text();
const $resp = $(resp);
// the response is a full HTML page, we need to extract the relevant contents:
// 1. append the newly loaded file list items to the existing list
// * append the newly loaded file list items to the existing list
$('#diff-incomplete').replaceWith($resp.find('#diff-file-boxes').children());

onShowMoreFiles();
return true;
} catch (error) {
console.error('Error:', error);
showErrorToast('An error occurred while loading more files.');
} finally {
target?.classList.remove('disabled');
pageData.diffFileInfo.isLoadingNewData = false;
btn.classList.remove('disabled');
}
}

function initRepoDiffShowMore() {
$(document).on('click', 'a#diff-show-more-files', (e) => {
addDelegatedEventListener(document, 'click', 'a#diff-show-more-files', (el, e) => {
e.preventDefault();

const linkLoadMore = e.target.getAttribute('data-href');
loadMoreFiles(linkLoadMore);
loadMoreFiles(el);
});

$(document).on('click', 'a.diff-load-button', async (e) => {
Expand Down Expand Up @@ -232,70 +228,41 @@ function initRepoDiffShowMore() {
});
}

function shouldStopLoading() {
if (!window.location.hash) {
return true; // No hash to look for
}

// eslint-disable-next-line unicorn/prefer-query-selector
const targetElement = document.getElementById(window.location.hash.substring(1));
if (targetElement) {
return true; // The element is already loaded
}

return !pageData.diffFileInfo.isIncomplete;
}

// This is a flag to ensure that only one loadUntilFound is running at a time
let isLoadUntilFoundRunning = false;

async function loadUntilFound() {
// this ensures that only one loadUntilFound is running at a time
if (isLoadUntilFoundRunning === true) {
const hashTargetSelector = window.location.hash;
if (!hashTargetSelector.startsWith('#diff-') && !hashTargetSelector.startsWith('#issuecomment-')) {
return;
}
isLoadUntilFoundRunning = true;

try {
while (!shouldStopLoading()) {
const showMoreButton = document.querySelector('#diff-show-more-files');
if (!showMoreButton) {
console.error('Could not find the show more files button');
return;
}

const url = showMoreButton.getAttribute('data-href');
if (!url) {
console.error('Could not find the data-href attribute of the show more files button');
return;
}

// Load more files, await ensures we don't block progress
await loadMoreFiles(url);
while (true) {
// use getElementById to avoid querySelector throws an error when the hash is invalid
// eslint-disable-next-line unicorn/prefer-query-selector
const targetElement = document.getElementById(hashTargetSelector.substring(1));
if (targetElement) {
targetElement.scrollIntoView();
return;
}

if (window.location.hash) {
const targetElement = document.querySelector(window.location.hash);
if (targetElement) {
targetElement.scrollIntoView();
}
// the button will be refreshed after each "load more", so query it every time
const showMoreButton = document.querySelector('#diff-show-more-files');
if (!showMoreButton) {
return; // nothing more to load
}
} finally {
isLoadUntilFoundRunning = false;

// Load more files, await ensures we don't block progress
const ok = await loadMoreFiles(showMoreButton);
if (!ok) return; // failed to load more files
}
}

function initRepoDiffHashChangeListener() {
const el = document.querySelector('#diff-file-tree');
if (!el) return;

window.addEventListener('hashchange', loadUntilFound);
loadUntilFound();
}

export function initRepoDiffView() {
initRepoDiffConversationForm();
if (!$('#diff-file-list').length) return;
if (!$('#diff-file-boxes').length) return;
initDiffFileTree();
initDiffCommitSelect();
initRepoDiffShowMore();
Expand Down
9 changes: 7 additions & 2 deletions web_src/js/modules/stores.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
import {reactive} from 'vue';
import type {Reactive} from 'vue';

const {pageData} = window.config;

let diffTreeStoreReactive: Reactive<Record<string, any>>;
export function diffTreeStore() {
if (!diffTreeStoreReactive) {
diffTreeStoreReactive = reactive(window.config.pageData.diffFileInfo);
window.config.pageData.diffFileInfo = diffTreeStoreReactive;
diffTreeStoreReactive = reactive({
files: pageData.DiffFiles,
fileTreeIsVisible: false,
selectedItem: '',
});
}
return diffTreeStoreReactive;
}
Loading