Add new command: openCollapsed #125

jacekkopecky · 2025-09-28T14:01:49Z

The aim of this PR is mainly to address #124, but also includes fixes that could be added independent of tackling #124.

The new command looks like this:

The other changes included are:

fix for an issue: when a diff has some files marked as viewed, and we move to a different tab and then back to the diff tab, the view updates and unnecessarily scrolls to the last viewed file. My change replicates diff2html functionality in order to avoid using element.click() which focuses on it, and scrolls to it.
and smaller clarifications and cleanups

@caponetto any comments are welcome; I think this is best reviewed commit-by-commit.

caponetto · 2025-10-03T14:18:41Z

Thanks for the PR @jacekkopecky! I'll take a look at it asap

Copilot

Pull Request Overview

Adds a new diffviewer.openCollapsed command to open diffs with all files initially collapsed (treated as viewed), and refactors the webview/update logic to support collapsing without triggering focus/scroll side effects. Also optimizes viewed state persistence (removing empty state) and replaces click-driven collapse behavior with explicit DOM class manipulation to avoid unintended scrolling.

New openCollapsed command and menu contribution
Introduces collapseAll payload flag and logic to pre-collapse all diff files
Replaces toggle.click() usage with manual DOM updates and adjusts viewed state saving

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/webview/message/handler.ts	Implements collapseAll flow, custom file collapse logic, revised viewed state messaging, and UI helper adjustments
src/webview/message/api.ts	Updates UpdateWebviewPayload: removes diffContainer, adds collapseAll flag
src/webview/css/elements.ts	Adds selector for viewed toggle label to support manual collapse styling
src/webview/css/classes.ts	Adds enum with diff2html CSS class names used when emulating collapse behavior
src/extension/viewed-state.ts	Avoids persisting empty viewed state by storing undefined
src/extension/provider.ts	Registers openCollapsed command and propagates collapseAll flag to webview
package.json	Contributes new command and adds it to the explorer context menu

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-03T14:20:51Z

src/webview/message/handler.ts

-      if (toggle.checked !== isChecked) {
-        toggle.click();
-      }
+      this.updateDiff2HtmlFileCollapsed(toggle, viewed);


When marking all files as viewed (viewed === true), CHANGED_SINCE_VIEWED is not removed from toggles, whereas the single-toggle path removes it in onViewedToggleChangedHandler. This leads to stale 'changed since viewed' indicators after 'mark all' operations. Remove the class when viewed is true, e.g. toggle.classList.remove(CHANGED_SINCE_VIEWED) inside the loop.

Suggested change

this.updateDiff2HtmlFileCollapsed(toggle, viewed);

this.updateDiff2HtmlFileCollapsed(toggle, viewed);

if (viewed) {

toggle.classList.remove(CHANGED_SINCE_VIEWED);

}

can be done unconditionally

Copilot · 2025-10-03T14:20:52Z

src/webview/message/handler.ts

+      const diffHash = await this.getDiffHash(toggle);
+      if (diffHash !== oldSha1) {
+        this.updateDiff2HtmlFileCollapsed(toggle, false);
+        toggle.classList.add(CHANGED_SINCE_VIEWED);


For unchanged files (diffHash === oldSha1) the previous behavior (toggle.click()) would clear CHANGED_SINCE_VIEWED via onViewedToggleChangedHandler; now the class is never removed if it was previously present. Explicitly remove CHANGED_SINCE_VIEWED for the unchanged (still-collapsed) case to avoid stale indicators.

Suggested change

toggle.classList.add(CHANGED_SINCE_VIEWED);

toggle.classList.add(CHANGED_SINCE_VIEWED);

} else {

toggle.classList.remove(CHANGED_SINCE_VIEWED);

unnecessary because this code is only called after a rerender so the class isn't there

Copilot · 2025-10-03T14:20:52Z

src/webview/message/handler.ts

+
+    // first synchronously hide all viewed files so we don't render too much in case the file is huge and most is collapsed
+    // we'll do a second pass over hidden files, so we'll hold them in togglesToRevisit
+    const togglesToRevisit = [];


[nitpick] togglesToRevisit is inferred as any[] (empty array literal) until first push; under strict/noImplicitAny this may degrade type safety. Add an explicit type: const togglesToRevisit: { toggle: HTMLInputElement; oldSha1: string }[] = [];.

Suggested change

const togglesToRevisit = [];

const togglesToRevisit: { toggle: HTMLInputElement; oldSha1: string }[] = [];

Copilot · 2025-10-03T14:20:52Z

src/webview/message/handler.ts

-      }
+      this.updateDiff2HtmlFileCollapsed(toggle, viewed);
+      // no await here so we synchronously return from this function, the sending will happen later as hashes get computed
+      this.sendFileViewedMessage(toggle, viewed);


[nitpick] sendFileViewedMessage returns a Promise but the result is neither awaited nor intentionally ignored; if it rejects it could cause an unhandled rejection. Consider prefixing with void or adding a .catch(...) to document intentional fire-and-forget behavior: void this.sendFileViewedMessage(toggle, viewed);.

Suggested change

this.sendFileViewedMessage(toggle, viewed);

void this.sendFileViewedMessage(toggle, viewed);

Copilot · 2025-10-03T14:20:53Z

src/extension/provider.ts

      vscode.commands.registerCommand("diffviewer.showLineByLine", () => setOutputFormatConfig("line-by-line")),
      vscode.commands.registerCommand("diffviewer.showSideBySide", () => setOutputFormatConfig("side-by-side")),
+      vscode.commands.registerCommand("diffviewer.openCollapsed", async (file) => {
+        const collapsedUri = vscode.Uri.from({ ...file, query: "collapsed" });


[nitpick] Use the built-in URI.with API instead of spreading the URI object to preserve all internal fields safely: const collapsedUri = file.with({ query: 'collapsed' });.

Suggested change

const collapsedUri = vscode.Uri.from({ ...file, query: "collapsed" });

const collapsedUri = file.with({ query: "collapsed" });

jacekkopecky · 2025-10-03T16:30:23Z

Thanks @caponetto ; I can look at the AI's suggestions tomorrow, it seems I really missed something but I'm not sure about how the AI suggests to fix it.

caponetto · 2025-10-04T11:25:16Z

@jacekkopecky Take these comments only as suggestions. Ignore them if they don't make sense.
Mind to rebase your PR with main? I've added a bunch of tests.

jacekkopecky · 2025-10-04T14:23:17Z

@jacekkopecky Take these comments only as suggestions. Ignore them if they don't make sense.

Of course. 😄

Mind to rebase your PR with main? I've added a bunch of tests.

Happy to.

The webView already uses SkeletonElementIds and we never use a different payload.diffContainer value anyway.

isVisible is a bit ambiguous - we're making the main body *not* visible and the loading messages visible. Then a consistent renaming for showEmpty().

This makes the workspace storage file a bit smaller.

When we use toggle.click(), that toggle gets focused, which causes the window to scroll to the last collapsed file when we switch to that tab. That's disruptive and this fixes it.

This command is in the context menu for diff files in the explorer. It opens the diff file with all parts collapsed (marked as viewed) to potentially make it easier to deal with very large diff files.

This omission was spotted by CoPilot

codecov · 2025-10-04T21:05:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/extension/provider.ts	`91.17% <100.00%> (+0.40%)`	⬆️
src/extension/viewed-state.ts	`100.00% <100.00%> (ø)`
src/webview/css/classes.ts	`100.00% <100.00%> (ø)`
src/webview/css/elements.ts	`100.00% <100.00%> (ø)`
src/webview/message/handler.ts	`91.58% <100.00%> (+1.52%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The previous test would still pass if the two commands used the opposite setOutputFormatConfig calls.

jacekkopecky · 2025-10-04T22:07:41Z

@caponetto I think everything is handled now. I hope I got the tests right, your way of mocking things is different from what I'm used to…

jacekkopecky mentioned this pull request Sep 28, 2025

Very Slow on large-ish diff files #124

Open

caponetto requested a review from Copilot October 3, 2025 14:18

Copilot AI reviewed Oct 3, 2025

View reviewed changes

jacekkopecky added 8 commits October 4, 2025 16:08

Remove unnecessary runtime parameter

832a21e

The webView already uses SkeletonElementIds and we never use a different payload.diffContainer value anyway.

Renaming for clarity

5a00da7

isVisible is a bit ambiguous - we're making the main body *not* visible and the loading messages visible. Then a consistent renaming for showEmpty().

Remove state objects when empty

7b95b1a

This makes the workspace storage file a bit smaller.

Fix: Collapse viewed files without scrolling

47d57b4

When we use toggle.click(), that toggle gets focused, which causes the window to scroll to the last collapsed file when we switch to that tab. That's disruptive and this fixes it.

Add diffviewer.openCollapsed command

377efe7

This command is in the context menu for diff files in the explorer. It opens the diff file with all parts collapsed (marked as viewed) to potentially make it easier to deal with very large diff files.

Make tests pass

7394e87

Fix CHANGED_SINCE_VIEWED behaviour on toggling all

1353029

This omission was spotted by CoPilot

🎨 Implement two useful nit-picks from CoPilot

48893b7

jacekkopecky force-pushed the 124-open-collapsed branch from 7f2e1f3 to 48893b7 Compare October 4, 2025 21:02

Fix test

cd3d360

jacekkopecky added 4 commits October 4, 2025 22:51

Fix test running with vscode jest extension

4b38676

Clarify test

8a4e0a1

The previous test would still pass if the two commands used the opposite setOutputFormatConfig calls.

Test the new command

ac5bb1f

Test collapseAll

6648e41

	const togglesToRevisit = [];
	const togglesToRevisit: { toggle: HTMLInputElement; oldSha1: string }[] = [];

	this.sendFileViewedMessage(toggle, viewed);
	void this.sendFileViewedMessage(toggle, viewed);

	const collapsedUri = vscode.Uri.from({ ...file, query: "collapsed" });
	const collapsedUri = file.with({ query: "collapsed" });

Add new command: openCollapsed #125

Are you sure you want to change the base?

Add new command: openCollapsed #125

Uh oh!

Conversation

jacekkopecky commented Sep 28, 2025

Uh oh!

caponetto commented Oct 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

jacekkopecky Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

jacekkopecky Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

jacekkopecky commented Oct 3, 2025

Uh oh!

caponetto commented Oct 4, 2025

Uh oh!

jacekkopecky commented Oct 4, 2025

Uh oh!

codecov bot commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jacekkopecky commented Oct 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 4, 2025 •

edited

Loading