Skip to content

Add full-file expansion in diffs - #36663

Draft
silverwind wants to merge 22 commits into
go-gitea:mainfrom
silverwind:fullexpand
Draft

Add full-file expansion in diffs#36663
silverwind wants to merge 22 commits into
go-gitea:mainfrom
silverwind:fullexpand

Conversation

@silverwind

@silverwind silverwind commented Feb 17, 2026

Copy link
Copy Markdown
Member

New button in diff file headers to expand all collapsed sections in one click.

image

Changes

Backend:

  • ExcerptBlob returns JSON for both single and batch expand
  • excerptBlobExpandAll: single batch request reads blob once, highlights once via shared diffVarMutable pointers
  • MaxDisplayFileSize guard and 100-section cap to prevent OOM
  • parseBatchExcerptOptions uses FormStrings for repeated query params

Frontend:

  • Replace HTMX with unified JS fetch using data-global-click delegation
  • fetchBlobExcerpt: single-section expand with is-loading guard
  • onDiffExpandAll: batch expand via single request, collapse via saved innerHTML
  • Auto-flips expand-all button when all gaps are manually expanded

Tests:

  • E2e tests: expand-all, collapse, single expand, partial + batch, manual full expansion auto-flip

This PR was written with the help of Claude Opus 4.6

Add a per-file toggle button that expands all collapsed sections at once
and can revert to the original collapsed state. The expand mechanism uses
direct JS fetch instead of htmx, replacing the hx-get/hx-target attributes
on code expander buttons with data-url. Individual expand buttons are also
handled via JS delegated click handlers.

The expand-all uses direction=full to make the backend expand entire gaps
in a single response instead of chunk-by-chunk, significantly reducing
the number of network round-trips for large files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 17, 2026
@silverwind silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Feb 17, 2026
@silverwind
silverwind requested a review from Copilot February 17, 2026 20:56

This comment was marked as outdated.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: silverwind <me@silverwind.io>
Comment thread services/gitdiff/gitdiff.go Outdated
Comment thread web_src/js/features/repo-diff.ts Outdated
Comment thread web_src/js/features/repo-diff.ts Outdated
silverwind and others added 7 commits February 18, 2026 07:28
…ve null guards

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@silverwind

silverwind commented Feb 19, 2026

Copy link
Copy Markdown
Member Author

Small margin-left added to copy button to provide some separation with the text, this is consistent with GitHub. Screenshot in PR description is updated.

@wxiaoguang

Copy link
Copy Markdown
Contributor

// Loop: expand all collapsed sections until none remain.

For a file with 4000 lines (locale_en-US.json), each request expands 20 lines, then you sends 200 request to server in short time?

@silverwind

Copy link
Copy Markdown
Member Author

Yeah the current mechanism is inefficient. I guess we could do more backend changes to make it send all at once.

@wxiaoguang
wxiaoguang marked this pull request as draft February 19, 2026 09:42
silverwind and others added 2 commits February 20, 2026 04:55
When expanding all collapsed sections in a diff, the frontend now
collects per-gap parameters from all expander buttons, joins them with
commas, and makes a single GET request. The backend detects batch mode
(comma in last_left), renders each section to HTML, and returns a JSON
string array. Single-section expands are unchanged.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@silverwind
silverwind marked this pull request as ready for review February 20, 2026 05:23
@silverwind

silverwind commented Feb 20, 2026

Copy link
Copy Markdown
Member Author

Yeah the current mechanism is inefficient. I guess we could do more backend changes to make it send all at once.

Adressed in e643492.

@wxiaoguang

Copy link
Copy Markdown
Contributor

Horrible AI code, duplicate every line in backend.

@silverwind

silverwind commented Feb 20, 2026

Copy link
Copy Markdown
Member Author

Odd, I can make it deduplicate more, had already asked it to minimize the diff.

@silverwind silverwind mentioned this pull request Apr 10, 2026
@silverwind

silverwind commented Apr 10, 2026

Copy link
Copy Markdown
Member Author

Reworking this now. Plan is to replace the htmx-based fetch for single frequents with a json api that can return both single and all fragments as an array of html fragments. The full-file fetch will be a single fetch for all fragements in 1 call to the backend.

silverwind and others added 2 commits April 10, 2026 20:47
…e2e tests

Backend:
- ExcerptBlob returns JSON for both single and batch expand
- Add excerptBlobExpandAll with MaxDisplayFileSize guard and 100-section cap
- Use FormStrings for repeated query params instead of comma-separated
- BuildBlobExcerptDiffSectionsFull: shared diffVarMutable pointers for
  single-pass highlighting across all sections
- Keep ExcerptBlob nearly identical to main (4-line early return added)

Frontend:
- Replace HTMX with unified JS fetch using data-global-click delegation
- Remove htmx.process calls and sleep(500) hack
- fetchBlobExcerpt: single expand with is-loading guard
- onDiffExpandAll: batch expand via single request, collapse via saved
  innerHTML, auto-flip button when all gaps manually expanded
- State saved before is-loading class to avoid poisoning restored state

Tests:
- Add e2e tests: direct expand-all, collapse, single expand, partial
  manual + batch, manual full expansion auto-flip
- Remove old integration tests replaced by e2e coverage
- Update pull_commit_test for JSON response format

Co-Authored-By: Claude (Opus 4.6) <noreply@anthropic.com>
@silverwind silverwind changed the title Add full-file expansion in diff blocks Add full-file expansion in diffs Apr 10, 2026
@silverwind

silverwind commented Apr 10, 2026

Copy link
Copy Markdown
Member Author

Rewrite done:

  • htmx replaced with data-global-click
  • Backend logic condensed into excerptBlobExpandAll
  • Added E2E tests
  • All fetches are now JSON array of HTML instead of single HTML

@silverwind
silverwind marked this pull request as ready for review April 10, 2026 21:34
silverwind and others added 2 commits April 10, 2026 23:44
Co-Authored-By: Claude (Opus 4.6) <noreply@anthropic.com>
@silverwind
silverwind requested a review from wxiaoguang April 10, 2026 22:03
silverwind and others added 2 commits April 11, 2026 00:12
Co-Authored-By: Claude (Opus 4.6) <noreply@anthropic.com>
silverwind and others added 2 commits April 11, 2026 01:04
@silverwind

Copy link
Copy Markdown
Member Author

One more minor enhancement added: All expand buttons now set aria-label and the e2e tests use those labels to select them.

@wxiaoguang wxiaoguang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Horrible AI slop and duplicate code.

Need to rewrite most of them (if I would have some time, but I can't promise)

@silverwind

Copy link
Copy Markdown
Member Author

I backfed all your vague feedback into it when producing this. Next time give specific feedback what you want and we might get something you like.

@wxiaoguang

wxiaoguang commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Duplicate code, fragile logic, etc.

I don't want to spend more time on teaching AI on this topic, which would cost multiple times of time than I do it by myself (to be clear: I also use AI, and will be assisted by AI).

Indeed there are also workarounds: just click the "View File", and/or "Blame" the file. "Full-file expansion in diff" seems to be a rare use case during code review (in most cases, people don't really need to read the unchanged parts)

Since it is just a nice-to-have feature, it doesn't block any real world user and no requirement issue from end users, so either someone can make it overall right, or when I have some free time and get interest to clean up the AI slop.

@silverwind

silverwind commented Apr 11, 2026

Copy link
Copy Markdown
Member Author

Give clear actionable sentence or 2 how you want the design to look. That would have taken you less time than writing that last comment and would be actually helpful.

"Unclean code" without actionable feedback is not a valid reason for a rejection imho when the code demonstrably works, and has 100% test coverage.

The repo is full of unclean code because of years of careless rubberstamping, why should my PRs be treated differently now?

@wxiaoguang

wxiaoguang commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

The repo is full of unclean code because of years of careless rubberstamping, why should my PRs be treated differently now?

No, not "differently". I always block rubberstamp approvals of the PRs which still have problems, including this one.

image

@TheFox0x7 TheFox0x7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Take it with a grain of salt as I have no experience with this part of codebase:

  1. ExcerptBlob and excerptBlobExpandAll are basically the same function.
  2. IMO io.Reader should be passed down to the function instead read in the expandAll one. You get more consistent apis that way.

I'm also not sure if it wouldn't be easier to ask git for this content directly by telling git to expand context for the diff but I did not have an occasion to dig into gitea's compare systems.

Also I can't use this on files that have their diff hidden by default, despite being able to request that diff.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +154 to +166
// Determine line range: 1-indexed inclusive
startLine := opts.LastRight + 1
endLine := opts.RightIndex - 1 // between-hunk: stop before next hunk
if opts.LeftHunkSize == 0 && opts.RightHunkSize == 0 {
endLine = len(allLines) // EOF: expand to end of file
}

leftStart := opts.LastLeft + 1
var diffLines []*DiffLine
for lineNum := startLine; lineNum <= endLine; lineNum++ {
lineIdx := lineNum - 1 // 0-indexed
if lineIdx >= len(allLines) {
break
Comment on lines +288 to +295
btn.classList.add('is-loading', 'loading-icon-2px');
try {
const response = await GET(btn.getAttribute('data-url')!);
if (!response.ok) return;

tr.insertAdjacentHTML('afterend', await response.json());
tr.remove();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants