Skip to content

Added checkboxes for file reviewing #340

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
8 changes: 8 additions & 0 deletions src/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import { Logger } from './logger';
import { SearchJiraHelper } from './views/jira/searchJiraHelper';
import { featureFlagClientInitializedEvent } from './analytics';
import { openPullRequest } from './commands/bitbucket/pullRequest';
import { CheckboxStateManager } from './views/nodes/checkBoxStateManager';

const isDebuggingRegex = /^--(debug|inspect)\b(-brk\b|(?!-))=?/;
const ConfigTargetKey = 'configurationTarget';
Expand All @@ -87,6 +88,8 @@ export class Container {
static async initialize(context: ExtensionContext, config: IConfig, version: string) {
const analyticsEnv: string = this.isDebugging ? 'staging' : 'prod';

this._checkboxStateManager = new CheckboxStateManager(context);

this._analyticsClient = analyticsClient({
origin: 'desktop',
env: analyticsEnv,
Expand Down Expand Up @@ -531,4 +534,9 @@ export class Container {
public static get pmfStats() {
return this._pmfStats;
}

private static _checkboxStateManager: CheckboxStateManager;
public static get checkboxStateManager() {
return this._checkboxStateManager;
}
}
30 changes: 30 additions & 0 deletions src/views/BitbucketExplorer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,46 @@ import { DescriptionNode, PullRequestTitlesNode } from './pullrequest/pullReques
import { PullRequestNodeDataProvider } from './pullRequestNodeDataProvider';
import { RefreshTimer } from './RefreshTimer';
import { BitbucketActivityMonitor } from './BitbucketActivityMonitor';
import { PullRequestFilesNode } from './nodes/pullRequestFilesNode';
import { DirectoryNode } from './nodes/directoryNode';
import { TreeView } from 'vscode';
import { AbstractBaseNode } from './nodes/abstractBaseNode';

export abstract class BitbucketExplorer extends Explorer implements Disposable {
private _disposable: Disposable;

private monitor: BitbucketActivityMonitor | undefined;
private _refreshTimer: RefreshTimer;
private _onDidChangeTreeData = new vscode.EventEmitter<AbstractBaseNode | undefined>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move this to pr explorer section


protected newTreeView(): TreeView<AbstractBaseNode> | undefined {
super.newTreeView();
this.setupCheckboxHandling();
return this.treeView;
}

private setupCheckboxHandling(): void {
if (!this.treeView) {
return;
}
this.treeView.onDidChangeCheckboxState((event) => {
event.items.forEach(([item, state]) => {
const checked = state === vscode.TreeItemCheckboxState.Checked;
if (item instanceof PullRequestFilesNode || item instanceof DirectoryNode) {
item.checked = checked;
this._onDidChangeTreeData.fire(item);
}
});
});
}

constructor(protected ctx: BitbucketContext) {
super(() => this.dispose());

setTimeout(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why>

this.setupCheckboxHandling();
}, 50);

Container.context.subscriptions.push(configuration.onDidChange(this._onConfigurationChanged, this));

this._refreshTimer = new RefreshTimer(this.explorerEnabledConfiguration(), this.refreshConfiguration(), () =>
Expand Down
21 changes: 21 additions & 0 deletions src/views/nodes/FilesRootNode.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as vscode from 'vscode';
import { AbstractBaseNode } from './abstractBaseNode';

export class FilesRootNode extends AbstractBaseNode {
constructor(
private fileNodes: AbstractBaseNode[],
parent?: AbstractBaseNode,
) {
super(parent);
}

getTreeItem(): vscode.TreeItem {
const item = new vscode.TreeItem('Files', vscode.TreeItemCollapsibleState.Collapsed);
item.contextValue = 'files-root';
return item;
}

async getChildren(): Promise<AbstractBaseNode[]> {
return this.fileNodes;
}
}
64 changes: 64 additions & 0 deletions src/views/nodes/checkBoxStateManager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { Logger } from 'src/logger';
import vscode from 'vscode';
interface CheckboxState {
id: string;
timestamp: number;
}
export class CheckboxStateManager {
private readonly STATE_EXPIRY = 24 * 60 * 60 * 1000 * 30; // 30 days
private readonly STORAGE_KEY = 'bitbucket.viewedFiles';

constructor(private context: vscode.ExtensionContext) {
this.cleanup();
}

private async cleanup(): Promise<void> {
const states = this.context.workspaceState.get<CheckboxState[]>(this.STORAGE_KEY, []);
const now = Date.now();

const validStates = states.filter((state) => {
const isValid = now - state.timestamp < this.STATE_EXPIRY;
if (!isValid) {
Logger.debug(`Removing expired checkbox state: ${state.id}`);
}
return isValid;
});

await this.context.workspaceState.update(this.STORAGE_KEY, validStates);
Logger.debug(`Cleanup complete. Remaining states: ${validStates.length}`);
}

isChecked(id: string): boolean {
const states = this.context.workspaceState.get<CheckboxState[]>(this.STORAGE_KEY, []);
const state = states.find((state) => state.id === id);

if (state) {
if (Date.now() - state.timestamp >= this.STATE_EXPIRY) {
this.setChecked(id, false);
return false;
}
return true;
}
return false;
}

setChecked(id: string, checked: boolean): void {
const states = this.context.workspaceState.get<CheckboxState[]>(this.STORAGE_KEY, []);

if (checked) {
const existingIndex = states.findIndex((state) => state.id === id);
if (existingIndex !== -1) {
states.splice(existingIndex, 1);
}
states.push({ id, timestamp: Date.now() });
} else {
const index = states.findIndex((state) => state.id === id);
if (index !== -1) {
states.splice(index, 1);
}
}

this.context.workspaceState.update(this.STORAGE_KEY, states);
Logger.debug(`Checkbox state updated: ${id} = ${checked}`);
}
}
16 changes: 12 additions & 4 deletions src/views/nodes/commitNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,18 @@ export class CommitNode extends AbstractBaseNode {
//TODO: pass tasks if commit-level tasks exist
//TODO: if there is more than one parent, there should probably be a notification about diff ambiguity, unless I can figure
//out a way to resolve this
const children = await createFileChangesNodes(this.pr, paginatedComments, diffs, conflictedFiles, [], {
lhs: this.commit.parentHashes?.[0] ?? '', //The only time I can think of this being undefined is for an initial commit, but what should the parent be there?
rhs: this.commit.hash,
});
const children = await createFileChangesNodes(
this.pr,
paginatedComments,
diffs,
conflictedFiles,
[],
{
lhs: this.commit.parentHashes?.[0] ?? '', //The only time I can think of this being undefined is for an initial commit, but what should the parent be there?
rhs: this.commit.hash,
},
'commits',
);
return children;
} catch (e) {
Logger.debug('error fetching changed files', e);
Expand Down
116 changes: 107 additions & 9 deletions src/views/nodes/directoryNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,125 @@ import * as vscode from 'vscode';
import { PRDirectory } from '../pullrequest/diffViewHelper';
import { AbstractBaseNode } from './abstractBaseNode';
import { PullRequestFilesNode } from './pullRequestFilesNode';
import { Container } from 'src/container';
import { PullRequest } from 'src/bitbucket/model';
import * as crypto from 'crypto';

export class DirectoryNode extends AbstractBaseNode {
constructor(private directoryData: PRDirectory) {
isRootFilesDirectory: boolean | undefined;
constructor(
private directoryData: PRDirectory,
private section: 'files' | 'commits' = 'files',
private pr: PullRequest,
private commitHash?: string,
) {
super();
}

private _isDirectClick = false;

get directoryId(): string {
const prUrl = this.pr.data.url;
const prUrlPath = vscode.Uri.parse(prUrl).path;
const prId = prUrlPath.slice(prUrlPath.lastIndexOf('/') + 1);
const repoUrl = prUrl.slice(0, prUrl.indexOf('/pull-requests'));
const repoId = repoUrl.slice(repoUrl.lastIndexOf('/') + 1);
const dirPath = this.directoryData.dirPath;

const dirId =
this.section === 'commits'
? `repo-${repoId}-pr-${prId}-section-${this.section}-commit-${this.commitHash}-directory-${dirPath}`
: `repo-${repoId}-pr-${prId}-section-${this.section}-directory-${dirPath}`;
return crypto.createHash('md5').update(dirId).digest('hex');
}

private areAllChildrenChecked(): boolean {
const allFilesChecked = this.directoryData.files.every((file) => {
const fileNode = new PullRequestFilesNode(file, this.section, this.pr);
return fileNode.checked;
});

const allSubdirsChecked = Array.from(this.directoryData.subdirs.values()).every((subdir) => {
const subdirNode = new DirectoryNode(subdir, this.section, this.pr);
return subdirNode.checked;
});

return allFilesChecked && allSubdirsChecked;
}

set checked(value: boolean) {
// This is to avoid infinite loops when setting the checked state
if (this._isDirectClick) {
return;
}

try {
this._isDirectClick = true;
Container.checkboxStateManager.setChecked(this.directoryId, value);

this.directoryData.files.forEach((file) => {
const fileNode = new PullRequestFilesNode(file, this.section, this.pr);
Container.checkboxStateManager.setChecked(fileNode.fileId, value);
});
this.directoryData.subdirs.forEach((subdir) => {
const subdirNode = new DirectoryNode(subdir, this.section, this.pr);
Container.checkboxStateManager.setChecked(subdirNode.directoryId, value);
});
} finally {
this._isDirectClick = false;
}
}

get checked(): boolean {
const hasExplicitState = Container.checkboxStateManager.isChecked(this.directoryId);
if (!hasExplicitState) {
return this.areAllChildrenChecked();
}
return hasExplicitState;
}

async getTreeItem(): Promise<vscode.TreeItem> {
const item = new vscode.TreeItem(this.directoryData.name, vscode.TreeItemCollapsibleState.Expanded);
this.isRootFilesDirectory =
this.section === 'files' && this.directoryData.name === 'Files' && this.directoryData.dirPath === '';

const item = new vscode.TreeItem(
this.directoryData.name,
this.isRootFilesDirectory
? vscode.TreeItemCollapsibleState.Collapsed
: vscode.TreeItemCollapsibleState.Expanded,
);
item.tooltip = this.directoryData.name;
item.iconPath = vscode.ThemeIcon.Folder;

if (!this.isRootFilesDirectory) {
item.iconPath = vscode.ThemeIcon.Folder;
}

const allChecked = this.areAllChildrenChecked();

if (!this.isRootFilesDirectory) {
item.checkboxState = this.checked
? vscode.TreeItemCheckboxState.Checked
: vscode.TreeItemCheckboxState.Unchecked;
item.contextValue = `directory${allChecked ? '.checked' : ''}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

}

if (!this.isRootFilesDirectory) {
item.id = this.directoryId;
}

return item;
}

async getChildren(element?: AbstractBaseNode): Promise<AbstractBaseNode[]> {
async getChildren(): Promise<AbstractBaseNode[]> {
const fileNodes: AbstractBaseNode[] = this.directoryData.files.map(
(diffViewArg) => new PullRequestFilesNode(diffViewArg, this.section, this.pr),
);

const directoryNodes: DirectoryNode[] = Array.from(
this.directoryData.subdirs.values(),
(subdir) => new DirectoryNode(subdir),
(subdir) => new DirectoryNode(subdir, this.section, this.pr),
);
const fileNodes: AbstractBaseNode[] = this.directoryData.files.map(
(diffViewArg) => new PullRequestFilesNode(diffViewArg),
);
return fileNodes.concat(directoryNodes);

return [...directoryNodes, ...fileNodes];
}
}
45 changes: 42 additions & 3 deletions src/views/nodes/pullRequestFilesNode.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,50 @@
import path from 'path';
import * as vscode from 'vscode';
import { FileStatus } from '../../bitbucket/model';
import { FileStatus, PullRequest } from '../../bitbucket/model';
import { Commands } from '../../commands';
import { configuration } from '../../config/configuration';
import { Resources } from '../../resources';
import { DiffViewArgs } from '../pullrequest/diffViewHelper';
import { PullRequestContextValue } from '../pullrequest/pullRequestNode';
import { AbstractBaseNode } from './abstractBaseNode';
import { Container } from '../../container';
import { DirectoryNode } from './directoryNode';
import * as crypto from 'crypto';

export class PullRequestFilesNode extends AbstractBaseNode {
constructor(private diffViewData: DiffViewArgs) {
constructor(
private diffViewData: DiffViewArgs,
private section: 'files' | 'commits' = 'files',
private pr: PullRequest,
private commitHash?: string,
) {
super();
}

get fileId(): string {
const prUrl = this.pr.data.url;
const repoUrl = prUrl.slice(0, prUrl.indexOf('/pull-requests'));
const repoId = repoUrl.slice(repoUrl.lastIndexOf('/') + 1);

const fileId =
this.section === 'commits'
? `repo-${repoId}-pr-${this.pr.data.id}-section-${this.section}-commit-${this.commitHash}-file-${this.diffViewData.fileDisplayData.fileDisplayName}-${this.diffViewData.latestFileContentHash}`
: `repo-${repoId}-pr-${this.pr.data.id}-section-${this.section}-file-${this.diffViewData.fileDisplayData.fileDisplayName}-${this.diffViewData.latestFileContentHash}`;
return crypto.createHash('md5').update(fileId).digest('hex');
}

get checked(): boolean {
return Container.checkboxStateManager.isChecked(this.fileId);
}

set checked(value: boolean) {
Container.checkboxStateManager.setChecked(this.fileId, value);

if (this.getParent instanceof DirectoryNode) {
this.getTreeItem();
}
}

async getTreeItem(): Promise<vscode.TreeItem> {
const itemData = this.diffViewData.fileDisplayData;
let fileDisplayString = itemData.fileDisplayName;
Expand All @@ -23,14 +55,21 @@ export class PullRequestFilesNode extends AbstractBaseNode {
`${itemData.numberOfComments > 0 ? '💬 ' : ''}${fileDisplayString}`,
vscode.TreeItemCollapsibleState.None,
);

item.checkboxState = this.checked
? vscode.TreeItemCheckboxState.Checked
: vscode.TreeItemCheckboxState.Unchecked;

item.id = this.fileId;

item.tooltip = itemData.fileDisplayName;
item.command = {
command: Commands.ViewDiff,
title: 'Diff file',
arguments: this.diffViewData.diffArgs,
};

item.contextValue = PullRequestContextValue;
item.contextValue = `${PullRequestContextValue}${this.checked ? '.checked' : ''}`;
item.resourceUri = vscode.Uri.parse(`${itemData.prUrl}#chg-${itemData.fileDisplayName}`);
switch (itemData.fileDiffStatus) {
case FileStatus.ADDED:
Expand Down
Loading
Loading