Skip to content

Commit 9b49d96

Browse files
authored
No checkboxes in commits subtree (#9041)
Fixes #340498
1 parent b6d319e commit 9b49d96

4 files changed

Lines changed: 188 additions & 20 deletions

File tree

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import { default as assert } from 'assert';
7+
import { createSandbox, SinonSandbox, SinonStubbedInstance } from 'sinon';
8+
import * as vscode from 'vscode';
9+
import { ViewedState } from '../../../common/comment';
10+
import { GitChangeType } from '../../../common/file';
11+
import { GitHubRef } from '../../../common/githubRef';
12+
import { FILE_LIST_LAYOUT } from '../../../common/settingKeys';
13+
import { OctokitCommon } from '../../../github/common';
14+
import { FolderRepositoryManager } from '../../../github/folderRepositoryManager';
15+
import { PullRequestModel } from '../../../github/pullRequestModel';
16+
import { GitFileChangeModel } from '../../../view/fileChangeModel';
17+
import { CommitNode } from '../../../view/treeNodes/commitNode';
18+
import { DirectoryTreeNode } from '../../../view/treeNodes/directoryTreeNode';
19+
import { FileChangeNode, GitFileChangeNode } from '../../../view/treeNodes/fileChangeNode';
20+
import { BaseTreeNode, TreeNode } from '../../../view/treeNodes/treeNode';
21+
import { MockRepository } from '../../mocks/mockRepository';
22+
import { mockTreeViewWorkbench } from '../../mocks/mockTreeViewWorkbench';
23+
24+
function createCommit(sha: string): OctokitCommon.PullsListCommitsResponseItem {
25+
return {
26+
sha,
27+
node_id: sha,
28+
url: '',
29+
html_url: '',
30+
comments_url: '',
31+
author: null,
32+
committer: null,
33+
parents: [],
34+
commit: {
35+
message: 'Test commit',
36+
author: null,
37+
committer: null,
38+
url: '',
39+
comment_count: 0,
40+
tree: { sha, url: '' },
41+
verification: { verified: false, reason: 'unsigned', signature: null, payload: null, verified_at: null },
42+
},
43+
};
44+
}
45+
46+
describe('CommitNode checkboxes', function () {
47+
let sinon: SinonSandbox;
48+
let manager: FolderRepositoryManager & SinonStubbedInstance<FolderRepositoryManager>;
49+
let pullRequest: PullRequestModel & SinonStubbedInstance<PullRequestModel>;
50+
let parent: BaseTreeNode;
51+
let nodes: TreeNode[];
52+
let layout: string;
53+
54+
beforeEach(function () {
55+
sinon = createSandbox();
56+
mockTreeViewWorkbench(sinon);
57+
nodes = [];
58+
layout = 'tree';
59+
const configuration: vscode.WorkspaceConfiguration = {
60+
get: sinon.stub().callsFake((key: string, fallback?: unknown) => key === FILE_LIST_LAYOUT ? layout : fallback),
61+
has: sinon.stub().returns(false),
62+
inspect: sinon.stub().returns(undefined),
63+
update: sinon.stub().rejects(new Error('Tests must not write settings')),
64+
};
65+
sinon.stub(vscode.workspace, 'getConfiguration').returns(configuration);
66+
manager = sinon.createStubInstance(FolderRepositoryManager) as typeof manager;
67+
sinon.stub(manager, 'repository').get(() => new MockRepository());
68+
pullRequest = sinon.createStubInstance(PullRequestModel) as typeof pullRequest;
69+
pullRequest.head = new GitHubRef('main', 'owner:main', 'head', 'https://github.com/owner/repo.git', 'owner', 'repo', false);
70+
sinon.stub(pullRequest, 'fileChangeViewedState').get(() => ({}));
71+
sinon.stub(pullRequest, 'reviewThreadsCache').get(() => []);
72+
pullRequest.onDidChangeReviewThreads = sinon.stub<Parameters<PullRequestModel['onDidChangeReviewThreads']>, vscode.Disposable>().returns(new vscode.Disposable(() => { }));
73+
pullRequest.onDidChangeFileViewedState = sinon.stub<Parameters<PullRequestModel['onDidChangeFileViewedState']>, vscode.Disposable>().returns(new vscode.Disposable(() => { }));
74+
parent = {
75+
refresh: sinon.stub(),
76+
reveal: sinon.stub().resolves(),
77+
children: undefined,
78+
view: vscode.window.createTreeView<TreeNode>('test', {
79+
treeDataProvider: {
80+
getTreeItem: node => node.getTreeItem(),
81+
getChildren: () => [],
82+
},
83+
}),
84+
};
85+
});
86+
87+
afterEach(function () {
88+
nodes.forEach(node => node.dispose());
89+
parent.view.dispose();
90+
sinon.restore();
91+
});
92+
93+
async function assertNoCheckboxes(children: TreeNode[]): Promise<{ files: number; directories: number }> {
94+
let files = 0;
95+
let directories = 0;
96+
for (const child of children) {
97+
nodes.push(child);
98+
assert.strictEqual((await child.getTreeItem()).checkboxState, undefined);
99+
if (child instanceof DirectoryTreeNode) {
100+
directories++;
101+
child.updateCheckboxFromChildren();
102+
assert.strictEqual(child.checkboxState, undefined);
103+
const descendants = await assertNoCheckboxes(await child.getChildren());
104+
files += descendants.files;
105+
directories += descendants.directories;
106+
} else {
107+
assert.ok(child instanceof FileChangeNode);
108+
files++;
109+
for (const state of [ViewedState.VIEWED, ViewedState.UNVIEWED]) {
110+
child.updateViewed(state);
111+
assert.strictEqual(child.getTreeItem().checkboxState, undefined);
112+
}
113+
}
114+
}
115+
return { files, directories };
116+
}
117+
118+
for (const sha of ['head', 'older']) {
119+
for (const testCase of [
120+
{ layout: 'tree', name: 'nested directories and root files', files: ['root.ts', 'src/a.ts', 'src/utils/b.ts', 'test/unit/c.ts'], directories: 3 },
121+
{ layout: 'tree', name: 'compacted directories', files: ['src/nested/a.ts', 'src/nested/utils/b.ts'], directories: 2 },
122+
{ layout: 'tree', name: 'root files only', files: ['a.ts', 'b.ts'], directories: 0 },
123+
{ layout: 'flat', name: 'flat files', files: ['root.ts', 'src/a.ts', 'src/utils/b.ts'], directories: 0 },
124+
]) {
125+
it(`has no checkboxes for ${testCase.name} in the ${sha} commit`, async function () {
126+
layout = testCase.layout;
127+
pullRequest.getCommitChangedFiles.resolves(testCase.files.map(filename => ({
128+
filename,
129+
sha,
130+
status: 'modified',
131+
additions: 1,
132+
deletions: 0,
133+
changes: 1,
134+
blob_url: '',
135+
raw_url: '',
136+
contents_url: '',
137+
})));
138+
const node = new CommitNode(parent, manager, pullRequest, createCommit(sha), sha === 'head');
139+
nodes.push(node);
140+
assert.strictEqual((await node.getTreeItem()).checkboxState, undefined);
141+
const counts = await assertNoCheckboxes(await node.getChildren());
142+
assert.deepStrictEqual(counts, { files: testCase.files.length, directories: testCase.directories });
143+
});
144+
}
145+
}
146+
147+
it('preserves file and directory checkboxes outside the commits tree', function () {
148+
pullRequest.isResolved.returns(true);
149+
assert.ok(pullRequest.isResolved());
150+
const directory = new DirectoryTreeNode(parent, 'src');
151+
const uri = vscode.Uri.joinPath(manager.repository.rootUri, 'src/a.ts');
152+
const model = new GitFileChangeModel(manager, pullRequest, {
153+
status: GitChangeType.MODIFY,
154+
fileName: 'src/a.ts',
155+
blobUrl: undefined,
156+
}, uri, uri, 'head');
157+
const file = new GitFileChangeNode(directory, manager, pullRequest, model);
158+
directory._children.push(file);
159+
nodes.push(directory, file);
160+
161+
file.getTreeItem();
162+
directory.getTreeItem();
163+
assert.strictEqual(file.checkboxState?.state, vscode.TreeItemCheckboxState.Unchecked);
164+
assert.strictEqual(directory.checkboxState?.state, vscode.TreeItemCheckboxState.Unchecked);
165+
file.updateViewed(ViewedState.VIEWED);
166+
directory.getTreeItem();
167+
assert.strictEqual(file.checkboxState?.state, vscode.TreeItemCheckboxState.Checked);
168+
assert.strictEqual(directory.checkboxState?.state, vscode.TreeItemCheckboxState.Checked);
169+
});
170+
});

‎src/view/treeNodes/commitNode.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,9 @@ export class CommitNode extends TreeNode implements vscode.TreeItem {
101101
this.pullRequestManager,
102102
this.pullRequest as (PullRequestModel & IResolvedPullRequestModel),
103103
changeModel,
104-
this.isCurrent
104+
this.isCurrent,
105+
undefined,
106+
false
105107
);
106108

107109
fileChangeNode.useViewChangesCommand();
@@ -113,7 +115,7 @@ export class CommitNode extends TreeNode implements vscode.TreeItem {
113115
const layout = vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE).get<string>(FILE_LIST_LAYOUT);
114116
if (layout === 'tree') {
115117
// tree view
116-
const dirNode = new DirectoryTreeNode(this, '');
118+
const dirNode = new DirectoryTreeNode(this, '', false);
117119
fileChangeNodes.forEach(f => dirNode.addFile(f));
118120
dirNode.finalize();
119121
if (dirNode.label === '') {

‎src/view/treeNodes/directoryTreeNode.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ export class DirectoryTreeNode extends TreeNode implements vscode.TreeItem {
1313
private pathToChild: Map<string, DirectoryTreeNode> = new Map();
1414
public checkboxState?: { state: vscode.TreeItemCheckboxState, tooltip: string, accessibilityInformation: vscode.AccessibilityInformation };
1515

16-
constructor(parent: TreeNodeParent, label: string) {
16+
constructor(parent: TreeNodeParent, label: string, private readonly showCheckbox: boolean = true) {
1717
super(parent);
1818
this.label = label;
1919
this.collapsibleState = vscode.TreeItemCollapsibleState.Expanded;
@@ -110,7 +110,7 @@ export class DirectoryTreeNode extends TreeNode implements vscode.TreeItem {
110110

111111
let node = this.pathToChild.get(dir);
112112
if (!node) {
113-
node = new DirectoryTreeNode(this, dir);
113+
node = new DirectoryTreeNode(this, dir, this.showCheckbox);
114114
this.pathToChild.set(dir, node);
115115
this._children.push(node);
116116
}
@@ -138,7 +138,11 @@ export class DirectoryTreeNode extends TreeNode implements vscode.TreeItem {
138138
}
139139

140140
public updateCheckboxFromChildren(): void {
141-
this.setCheckboxState(this.allChildrenViewed());
141+
if (this.showCheckbox) {
142+
this.setCheckboxState(this.allChildrenViewed());
143+
} else {
144+
this.checkboxState = undefined;
145+
}
142146
}
143147

144148
getTreeItem(): vscode.TreeItem {

‎src/view/treeNodes/fileChangeNode.ts‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ export class FileChangeNode extends TreeNode implements vscode.TreeItem {
101101
parent: TreeNodeParent,
102102
protected readonly pullRequestManager: FolderRepositoryManager,
103103
public readonly pullRequest: PullRequestModel & IResolvedPullRequestModel,
104-
public readonly changeModel: FileChangeModel
104+
public readonly changeModel: FileChangeModel,
105+
private readonly showCheckbox: boolean = true
105106
) {
106107
super(parent);
107108
const viewed = this.pullRequest.fileChangeViewedState[this.changeModel.fileName] ?? ViewedState.UNVIEWED;
@@ -155,22 +156,12 @@ export class FileChangeNode extends TreeNode implements vscode.TreeItem {
155156
}
156157
}
157158

158-
/**
159-
* Check if this file node is under a commit node in the tree hierarchy.
160-
* Files under commit nodes should not have checkboxes.
161-
*/
162-
private isUnderCommitNode(): boolean {
163-
// If the file's sha is different from the PR's head sha, it's from an older commit
164-
// and should not have a checkbox
165-
return this.changeModel.sha !== undefined && this.changeModel.sha !== this.pullRequest.head?.sha;
166-
}
167-
168159
updateViewed(viewed: ViewedState) {
169160
this.changeModel.updateViewed(viewed);
170161
this.contextValue = `${Schemes.FileChange}:${GitChangeType[this.changeModel.status]}:${viewed === ViewedState.VIEWED ? 'viewed' : 'unviewed'
171162
}`;
172-
// Don't show checkboxes for files under commit nodes
173-
if (!this.isUnderCommitNode()) {
163+
const isOlderCommit = this.changeModel.sha !== undefined && this.changeModel.sha !== this.pullRequest.head?.sha;
164+
if (this.showCheckbox && !isOlderCommit) {
174165
this.checkboxState = viewed === ViewedState.VIEWED ?
175166
{ state: vscode.TreeItemCheckboxState.Checked, tooltip: vscode.l10n.t('Mark File as Unviewed'), accessibilityInformation: { label: vscode.l10n.t('Mark file {0} as unviewed', this.label ?? '') } } :
176167
{ state: vscode.TreeItemCheckboxState.Unchecked, tooltip: vscode.l10n.t('Mark File as Viewed'), accessibilityInformation: { label: vscode.l10n.t('Mark file {0} as viewed', this.label ?? '') } };
@@ -315,9 +306,10 @@ export class GitFileChangeNode extends FileChangeNode implements vscode.TreeItem
315306
pullRequest: PullRequestModel & IResolvedPullRequestModel,
316307
changeModel: GitFileChangeModel,
317308
private isCurrent?: boolean,
318-
private _comments?: IComment[]
309+
private _comments?: IComment[],
310+
showCheckbox: boolean = true
319311
) {
320-
super(parent, pullRequestManager, pullRequest, changeModel);
312+
super(parent, pullRequestManager, pullRequest, changeModel, showCheckbox);
321313
}
322314

323315
get comments(): IComment[] {

0 commit comments

Comments
 (0)