From b0b128ad4bbab3ee7f4456837d8bcfb50d9375ae Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 8 Oct 2026 17:11:40 +0200 Subject: [PATCH] No checkboxes in commits subtree Fixes #340498 --- src/test/view/treeNodes/commitNode.test.ts | 170 +++++++++++++++++++++ src/view/treeNodes/commitNode.ts | 6 +- src/view/treeNodes/directoryTreeNode.ts | 10 +- src/view/treeNodes/fileChangeNode.ts | 22 +-- 4 files changed, 188 insertions(+), 20 deletions(-) create mode 100644 src/test/view/treeNodes/commitNode.test.ts diff --git a/src/test/view/treeNodes/commitNode.test.ts b/src/test/view/treeNodes/commitNode.test.ts new file mode 100644 index 0000000000..12b92a95ea --- /dev/null +++ b/src/test/view/treeNodes/commitNode.test.ts @@ -0,0 +1,170 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { default as assert } from 'assert'; +import { createSandbox, SinonSandbox, SinonStubbedInstance } from 'sinon'; +import * as vscode from 'vscode'; +import { ViewedState } from '../../../common/comment'; +import { GitChangeType } from '../../../common/file'; +import { GitHubRef } from '../../../common/githubRef'; +import { FILE_LIST_LAYOUT } from '../../../common/settingKeys'; +import { OctokitCommon } from '../../../github/common'; +import { FolderRepositoryManager } from '../../../github/folderRepositoryManager'; +import { PullRequestModel } from '../../../github/pullRequestModel'; +import { GitFileChangeModel } from '../../../view/fileChangeModel'; +import { CommitNode } from '../../../view/treeNodes/commitNode'; +import { DirectoryTreeNode } from '../../../view/treeNodes/directoryTreeNode'; +import { FileChangeNode, GitFileChangeNode } from '../../../view/treeNodes/fileChangeNode'; +import { BaseTreeNode, TreeNode } from '../../../view/treeNodes/treeNode'; +import { MockRepository } from '../../mocks/mockRepository'; +import { mockTreeViewWorkbench } from '../../mocks/mockTreeViewWorkbench'; + +function createCommit(sha: string): OctokitCommon.PullsListCommitsResponseItem { + return { + sha, + node_id: sha, + url: '', + html_url: '', + comments_url: '', + author: null, + committer: null, + parents: [], + commit: { + message: 'Test commit', + author: null, + committer: null, + url: '', + comment_count: 0, + tree: { sha, url: '' }, + verification: { verified: false, reason: 'unsigned', signature: null, payload: null, verified_at: null }, + }, + }; +} + +describe('CommitNode checkboxes', function () { + let sinon: SinonSandbox; + let manager: FolderRepositoryManager & SinonStubbedInstance; + let pullRequest: PullRequestModel & SinonStubbedInstance; + let parent: BaseTreeNode; + let nodes: TreeNode[]; + let layout: string; + + beforeEach(function () { + sinon = createSandbox(); + mockTreeViewWorkbench(sinon); + nodes = []; + layout = 'tree'; + const configuration: vscode.WorkspaceConfiguration = { + get: sinon.stub().callsFake((key: string, fallback?: unknown) => key === FILE_LIST_LAYOUT ? layout : fallback), + has: sinon.stub().returns(false), + inspect: sinon.stub().returns(undefined), + update: sinon.stub().rejects(new Error('Tests must not write settings')), + }; + sinon.stub(vscode.workspace, 'getConfiguration').returns(configuration); + manager = sinon.createStubInstance(FolderRepositoryManager) as typeof manager; + sinon.stub(manager, 'repository').get(() => new MockRepository()); + pullRequest = sinon.createStubInstance(PullRequestModel) as typeof pullRequest; + pullRequest.head = new GitHubRef('main', 'owner:main', 'head', 'https://github.com/owner/repo.git', 'owner', 'repo', false); + sinon.stub(pullRequest, 'fileChangeViewedState').get(() => ({})); + sinon.stub(pullRequest, 'reviewThreadsCache').get(() => []); + pullRequest.onDidChangeReviewThreads = sinon.stub, vscode.Disposable>().returns(new vscode.Disposable(() => { })); + pullRequest.onDidChangeFileViewedState = sinon.stub, vscode.Disposable>().returns(new vscode.Disposable(() => { })); + parent = { + refresh: sinon.stub(), + reveal: sinon.stub().resolves(), + children: undefined, + view: vscode.window.createTreeView('test', { + treeDataProvider: { + getTreeItem: node => node.getTreeItem(), + getChildren: () => [], + }, + }), + }; + }); + + afterEach(function () { + nodes.forEach(node => node.dispose()); + parent.view.dispose(); + sinon.restore(); + }); + + async function assertNoCheckboxes(children: TreeNode[]): Promise<{ files: number; directories: number }> { + let files = 0; + let directories = 0; + for (const child of children) { + nodes.push(child); + assert.strictEqual((await child.getTreeItem()).checkboxState, undefined); + if (child instanceof DirectoryTreeNode) { + directories++; + child.updateCheckboxFromChildren(); + assert.strictEqual(child.checkboxState, undefined); + const descendants = await assertNoCheckboxes(await child.getChildren()); + files += descendants.files; + directories += descendants.directories; + } else { + assert.ok(child instanceof FileChangeNode); + files++; + for (const state of [ViewedState.VIEWED, ViewedState.UNVIEWED]) { + child.updateViewed(state); + assert.strictEqual(child.getTreeItem().checkboxState, undefined); + } + } + } + return { files, directories }; + } + + for (const sha of ['head', 'older']) { + for (const testCase of [ + { layout: 'tree', name: 'nested directories and root files', files: ['root.ts', 'src/a.ts', 'src/utils/b.ts', 'test/unit/c.ts'], directories: 3 }, + { layout: 'tree', name: 'compacted directories', files: ['src/nested/a.ts', 'src/nested/utils/b.ts'], directories: 2 }, + { layout: 'tree', name: 'root files only', files: ['a.ts', 'b.ts'], directories: 0 }, + { layout: 'flat', name: 'flat files', files: ['root.ts', 'src/a.ts', 'src/utils/b.ts'], directories: 0 }, + ]) { + it(`has no checkboxes for ${testCase.name} in the ${sha} commit`, async function () { + layout = testCase.layout; + pullRequest.getCommitChangedFiles.resolves(testCase.files.map(filename => ({ + filename, + sha, + status: 'modified', + additions: 1, + deletions: 0, + changes: 1, + blob_url: '', + raw_url: '', + contents_url: '', + }))); + const node = new CommitNode(parent, manager, pullRequest, createCommit(sha), sha === 'head'); + nodes.push(node); + assert.strictEqual((await node.getTreeItem()).checkboxState, undefined); + const counts = await assertNoCheckboxes(await node.getChildren()); + assert.deepStrictEqual(counts, { files: testCase.files.length, directories: testCase.directories }); + }); + } + } + + it('preserves file and directory checkboxes outside the commits tree', function () { + pullRequest.isResolved.returns(true); + assert.ok(pullRequest.isResolved()); + const directory = new DirectoryTreeNode(parent, 'src'); + const uri = vscode.Uri.joinPath(manager.repository.rootUri, 'src/a.ts'); + const model = new GitFileChangeModel(manager, pullRequest, { + status: GitChangeType.MODIFY, + fileName: 'src/a.ts', + blobUrl: undefined, + }, uri, uri, 'head'); + const file = new GitFileChangeNode(directory, manager, pullRequest, model); + directory._children.push(file); + nodes.push(directory, file); + + file.getTreeItem(); + directory.getTreeItem(); + assert.strictEqual(file.checkboxState?.state, vscode.TreeItemCheckboxState.Unchecked); + assert.strictEqual(directory.checkboxState?.state, vscode.TreeItemCheckboxState.Unchecked); + file.updateViewed(ViewedState.VIEWED); + directory.getTreeItem(); + assert.strictEqual(file.checkboxState?.state, vscode.TreeItemCheckboxState.Checked); + assert.strictEqual(directory.checkboxState?.state, vscode.TreeItemCheckboxState.Checked); + }); +}); diff --git a/src/view/treeNodes/commitNode.ts b/src/view/treeNodes/commitNode.ts index d8918058a3..f3c4c885ad 100644 --- a/src/view/treeNodes/commitNode.ts +++ b/src/view/treeNodes/commitNode.ts @@ -101,7 +101,9 @@ export class CommitNode extends TreeNode implements vscode.TreeItem { this.pullRequestManager, this.pullRequest as (PullRequestModel & IResolvedPullRequestModel), changeModel, - this.isCurrent + this.isCurrent, + undefined, + false ); fileChangeNode.useViewChangesCommand(); @@ -113,7 +115,7 @@ export class CommitNode extends TreeNode implements vscode.TreeItem { const layout = vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE).get(FILE_LIST_LAYOUT); if (layout === 'tree') { // tree view - const dirNode = new DirectoryTreeNode(this, ''); + const dirNode = new DirectoryTreeNode(this, '', false); fileChangeNodes.forEach(f => dirNode.addFile(f)); dirNode.finalize(); if (dirNode.label === '') { diff --git a/src/view/treeNodes/directoryTreeNode.ts b/src/view/treeNodes/directoryTreeNode.ts index 4b0d6dfdeb..3641d3bad0 100644 --- a/src/view/treeNodes/directoryTreeNode.ts +++ b/src/view/treeNodes/directoryTreeNode.ts @@ -13,7 +13,7 @@ export class DirectoryTreeNode extends TreeNode implements vscode.TreeItem { private pathToChild: Map = new Map(); public checkboxState?: { state: vscode.TreeItemCheckboxState, tooltip: string, accessibilityInformation: vscode.AccessibilityInformation }; - constructor(parent: TreeNodeParent, label: string) { + constructor(parent: TreeNodeParent, label: string, private readonly showCheckbox: boolean = true) { super(parent); this.label = label; this.collapsibleState = vscode.TreeItemCollapsibleState.Expanded; @@ -110,7 +110,7 @@ export class DirectoryTreeNode extends TreeNode implements vscode.TreeItem { let node = this.pathToChild.get(dir); if (!node) { - node = new DirectoryTreeNode(this, dir); + node = new DirectoryTreeNode(this, dir, this.showCheckbox); this.pathToChild.set(dir, node); this._children.push(node); } @@ -138,7 +138,11 @@ export class DirectoryTreeNode extends TreeNode implements vscode.TreeItem { } public updateCheckboxFromChildren(): void { - this.setCheckboxState(this.allChildrenViewed()); + if (this.showCheckbox) { + this.setCheckboxState(this.allChildrenViewed()); + } else { + this.checkboxState = undefined; + } } getTreeItem(): vscode.TreeItem { diff --git a/src/view/treeNodes/fileChangeNode.ts b/src/view/treeNodes/fileChangeNode.ts index 4e1e8ac0c6..6950f59157 100644 --- a/src/view/treeNodes/fileChangeNode.ts +++ b/src/view/treeNodes/fileChangeNode.ts @@ -101,7 +101,8 @@ export class FileChangeNode extends TreeNode implements vscode.TreeItem { parent: TreeNodeParent, protected readonly pullRequestManager: FolderRepositoryManager, public readonly pullRequest: PullRequestModel & IResolvedPullRequestModel, - public readonly changeModel: FileChangeModel + public readonly changeModel: FileChangeModel, + private readonly showCheckbox: boolean = true ) { super(parent); const viewed = this.pullRequest.fileChangeViewedState[this.changeModel.fileName] ?? ViewedState.UNVIEWED; @@ -155,22 +156,12 @@ export class FileChangeNode extends TreeNode implements vscode.TreeItem { } } - /** - * Check if this file node is under a commit node in the tree hierarchy. - * Files under commit nodes should not have checkboxes. - */ - private isUnderCommitNode(): boolean { - // If the file's sha is different from the PR's head sha, it's from an older commit - // and should not have a checkbox - return this.changeModel.sha !== undefined && this.changeModel.sha !== this.pullRequest.head?.sha; - } - updateViewed(viewed: ViewedState) { this.changeModel.updateViewed(viewed); this.contextValue = `${Schemes.FileChange}:${GitChangeType[this.changeModel.status]}:${viewed === ViewedState.VIEWED ? 'viewed' : 'unviewed' }`; - // Don't show checkboxes for files under commit nodes - if (!this.isUnderCommitNode()) { + const isOlderCommit = this.changeModel.sha !== undefined && this.changeModel.sha !== this.pullRequest.head?.sha; + if (this.showCheckbox && !isOlderCommit) { this.checkboxState = viewed === ViewedState.VIEWED ? { state: vscode.TreeItemCheckboxState.Checked, tooltip: vscode.l10n.t('Mark File as Unviewed'), accessibilityInformation: { label: vscode.l10n.t('Mark file {0} as unviewed', this.label ?? '') } } : { 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 pullRequest: PullRequestModel & IResolvedPullRequestModel, changeModel: GitFileChangeModel, private isCurrent?: boolean, - private _comments?: IComment[] + private _comments?: IComment[], + showCheckbox: boolean = true ) { - super(parent, pullRequestManager, pullRequest, changeModel); + super(parent, pullRequestManager, pullRequest, changeModel, showCheckbox); } get comments(): IComment[] {