Batch mark/unmark files as viewed by joshuaobrien · Pull Request #4700 · microsoft/vscode-pull-request-github

joshuaobrien · 2023-04-03T12:46:49Z

Context

Previously, when marking/unmarking a folder as viewed, a request was sent for every single file. This PR ensures that only one request is sent when marking/unmarking a folder as viewed.

Fixes #4520

Before

batch-before.mp4

After

batch-after.mp4

src/github/pullRequestModel.ts

alexr00

Thank you for the PR! It looks like its heading in a very good direction! I have a few questions/comments.

src/github/pullRequestModel.ts

src/view/treeNodes/directoryTreeNode.ts

src/view/prsTreeDataProvider.ts

src/view/treeNodes/fileChangeNode.ts

joshuaobrien · 2023-04-26T01:42:34Z

I'm not a fan of this inlining. Is it possible to do without the inlining and instead have a new query in queries.gql

I spent a bit of time investigating this before raising this PR, but I couldn't find a way to do it. That said, I've never used graphql before outside of this project so I'm sure I'm missing something. I'll take another look this evening!

alexr00

Thanks for the updates! In the future, it would make reviewing PRs much easier/faster for me if you don't force push your branch. It's hard to tell which files have changed since my last review after a force push.

I tested this out and it doesn't seem to work. I get this error:

stack trace: GraphQLError: Syntax Error: Cannot parse the unexpected character ".".
    at syntaxError (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\error\syntaxError.js:15:1)
    at readToken (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\lexer.js:360:1)
    at Lexer.lookahead (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\lexer.js:75:1)
    at Lexer.advance (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\lexer.js:58:1)
    at Parser.expectToken (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:1408:1)
    at Parser.parseName (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:98:1)
    at Parser.parseField (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:295:1)
    at Parser.parseSelection (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:284:1)
    at Parser.many (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:1523:1)
    at Parser.parseSelectionSet (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:271:1)
    at Parser.parseOperationDefinition (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:199:1)
    at Parser.parseDefinition (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:137:1)
    at Parser.many (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:1523:1)
    at Parser.parseDocument (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:115:1)
    at parse (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:31:1)
    at parseDocument (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql-tag\src\index.js:129:1)
    at gql (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql-tag\src\index.js:170:1)
    at _PullRequestModel.markFilesAsViewed (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\src\github\pullRequestModel.ts:1629:23)
extensionHostProcess.js:105
unhandledRejection {"message":"Syntax Error: Cannot parse the unexpected character \".\".","locations":[{"line":2,"column":25}]}
extensionHostProcess.js:105
rejected promise not handled within 1 second: Syntax Error: Cannot parse the unexpected character ".".

src/view/treeNodes/directoryTreeNode.ts

joshuaobrien · 2023-05-15T12:10:00Z

Thanks for the updates! In the future, it would make reviewing PRs much easier/faster for me if you don't force push your branch. It's hard to tell which files have changed since my last review after a force push.

I tested this out and it doesn't seem to work. I get this error:

stack trace: GraphQLError: Syntax Error: Cannot parse the unexpected character ".".
    at syntaxError (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\error\syntaxError.js:15:1)
    at readToken (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\lexer.js:360:1)
    at Lexer.lookahead (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\lexer.js:75:1)
    at Lexer.advance (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\lexer.js:58:1)
    at Parser.expectToken (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:1408:1)
    at Parser.parseName (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:98:1)
    at Parser.parseField (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:295:1)
    at Parser.parseSelection (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:284:1)
    at Parser.many (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:1523:1)
    at Parser.parseSelectionSet (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:271:1)
    at Parser.parseOperationDefinition (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:199:1)
    at Parser.parseDefinition (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:137:1)
    at Parser.many (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:1523:1)
    at Parser.parseDocument (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:115:1)
    at parse (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql\language\parser.js:31:1)
    at parseDocument (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql-tag\src\index.js:129:1)
    at gql (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\node_modules\graphql-tag\src\index.js:170:1)
    at _PullRequestModel.markFilesAsViewed (d:\repos\Microsoft\vscode-pull-request-github-2\dist\webpack:\vscode-pull-request-github\src\github\pullRequestModel.ts:1629:23)
extensionHostProcess.js:105
unhandledRejection {"message":"Syntax Error: Cannot parse the unexpected character \".\".","locations":[{"line":2,"column":25}]}
extensionHostProcess.js:105
rejected promise not handled within 1 second: Syntax Error: Cannot parse the unexpected character ".".

Huh okay. I'll see if I can reproduce that locally. I'll push up a fix if I can.

…s viewed or not viewed

joshuaobrien · 2023-11-26T06:26:03Z

This completely dropped off my radar. I'll fix it up and send it back for review soon.

Given how long it's been, I rebased my branch rather than have a massive merge commit. This required a force push, sorry. I won't force push for any further changes.

alexr00 · 2023-11-28T10:44:37Z

@joshuaobrien very good timing, thanks for reviving the change! We are doing issue/pr clean up in December and your PR is at the top of my list of my list.

joshuaobrien · 2023-12-03T10:01:33Z

src/view/treeNodes/treeNode.ts


 	updateFromCheckboxChanged(_newState: vscode.TreeItemCheckboxState): void { }

+	static processCheckboxUpdates(checkboxUpdates: vscode.TreeCheckboxChangeEvent<TreeNode>) {


Probably not the ideal place for this. I wanted somewhere easily accessible for both prChangesTreeDataProvider and prsTreeDataProvider. Where would you recommend I put it?

There isn't an existing place that this fits nicely. Let's put it in a new treeUtils.ts file next to treeNode.ts.

joshuaobrien · 2023-12-03T10:01:56Z

src/view/treeNodes/treeNode.ts

+		});
+
+		if (checkedNodes.length > 0) {
+			const prModel = checkedNodes[0].pullRequest;


Is it safe to assume that all nodes will be associated with the same PR?

src/github/pullRequestModel.ts

alexr00

Thanks for making this change! In the interest of getting this PR merged today, I've pushed the changes I suggested in my comments.

alexr00 · 2023-12-12T10:55:05Z

src/github/pullRequestModel.ts

-
-		this.setFileViewedState(fileName, ViewedState.VIEWED, event);
-	}
+	private readonly BATCH_SIZE = 100;


nit: this can be a const outside of the class so that it can be inlined during compilation.

alexr00 · 2023-12-12T10:58:45Z

src/view/treeNodes/treeNode.ts

+		});
+
+		if (checkedNodes.length > 0) {
+			const prModel = checkedNodes[0].pullRequest;


alexr00 · 2023-12-12T11:06:18Z

src/view/treeNodes/treeNode.ts


 	updateFromCheckboxChanged(_newState: vscode.TreeItemCheckboxState): void { }

+	static processCheckboxUpdates(checkboxUpdates: vscode.TreeCheckboxChangeEvent<TreeNode>) {


There isn't an existing place that this fits nicely. Let's put it in a new treeUtils.ts file next to treeNode.ts.

src/github/pullRequestModel.ts

joshuaobrien · 2023-12-12T11:49:17Z

Thanks @alexr00!

joshuaobrien force-pushed the batch-view-unview-reqs branch from 3548060 to 36e6697 Compare April 3, 2023 12:47

joshuaobrien commented Apr 3, 2023

View reviewed changes

src/github/pullRequestModel.ts Outdated Show resolved Hide resolved

joshuaobrien force-pushed the batch-view-unview-reqs branch from 36e6697 to 739a5bb Compare April 3, 2023 12:49

alexr00 requested changes Apr 13, 2023

View reviewed changes

src/github/pullRequestModel.ts Outdated Show resolved Hide resolved

src/view/treeNodes/directoryTreeNode.ts Outdated Show resolved Hide resolved

src/view/prsTreeDataProvider.ts Outdated Show resolved Hide resolved

src/view/treeNodes/fileChangeNode.ts Outdated Show resolved Hide resolved

alexr00 added this to the April 2023 milestone Apr 13, 2023

sandy081 assigned alexr00 Apr 25, 2023

alexr00 removed this from the April 2023 milestone Apr 25, 2023

alexr00 added this to the May 2023 milestone Apr 26, 2023

joshuaobrien force-pushed the batch-view-unview-reqs branch from a743283 to 721e2ff Compare April 26, 2023 14:44

alexr00 requested changes May 9, 2023

View reviewed changes

src/view/treeNodes/directoryTreeNode.ts Outdated Show resolved Hide resolved

alexr00 modified the milestones: May 2023, Backlog May 30, 2023

joshuaobrien force-pushed the batch-view-unview-reqs branch from 4c619ba to 0ff6cd2 Compare November 26, 2023 06:12

joshuaobrien added 2 commits November 26, 2023 17:20

Ensure only a single request is fired when marking an entire folder a…

80a3287

…s viewed or not viewed

Perform mutation in prsTreeDataProvider

21e18b0

joshuaobrien force-pushed the batch-view-unview-reqs branch from 0ff6cd2 to 21e18b0 Compare November 26, 2023 06:23

joshuaobrien mentioned this pull request Nov 26, 2023

Remove a few unused variables 🌱 #5510

Merged

alexr00 modified the milestones: Backlog, December 2023 Nov 28, 2023

joshuaobrien added 7 commits December 3, 2023 16:14

Dont pass pr model to directory tree node

0190973

Remove performMutation argument from updateFromCheckboxChanged

cf98105

add static processCheckboxUpdates helper to tree node

83bdbc2

use static helper in prChangesTreeDataProvider and prsTreeDataProvider

5e2f22a

Dont make _pullRequestModel private

82d5f99

Reduce indentation in helper

75b7dc5

fix typo in helper

20112e4

joshuaobrien marked this pull request as draft December 3, 2023 06:09

joshuaobrien added 4 commits December 3, 2023 19:36

Use platform specific separator

efd4921

Dont mark files as viewed/unviewed individually anymore

dff03ca

unify the mark and unmark functions. add batching

af87e1a

fix typo

8da00bd

joshuaobrien commented Dec 3, 2023

View reviewed changes

joshuaobrien added 2 commits December 3, 2023 21:02

fix another typo

e44c246

fix yet another typo, add some comments

9a4dc25

joshuaobrien marked this pull request as ready for review December 3, 2023 10:20

joshuaobrien requested a review from alexr00 December 3, 2023 10:21

joshuaobrien commented Dec 3, 2023

View reviewed changes

src/github/pullRequestModel.ts Show resolved Hide resolved

joaomoreno removed this from the December 2023 milestone Dec 11, 2023

Make suggested changes

641aa17

alexr00 added this to the December / January 2024 milestone Dec 12, 2023

alexr00 approved these changes Dec 12, 2023

View reviewed changes

alexr00 enabled auto-merge (squash) December 12, 2023 11:48

jrieken approved these changes Dec 12, 2023

View reviewed changes

alexr00 merged commit b888b3f into microsoft:main Dec 12, 2023


		updateFromCheckboxChanged(_newState: vscode.TreeItemCheckboxState): void { }

		static processCheckboxUpdates(checkboxUpdates: vscode.TreeCheckboxChangeEvent<TreeNode>) {

Conversation

joshuaobrien commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Before

After

Uh oh!

Uh oh!

alexr00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshuaobrien commented Apr 26, 2023

Uh oh!

alexr00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshuaobrien commented May 15, 2023

Uh oh!

joshuaobrien commented Nov 26, 2023

Uh oh!

alexr00 commented Nov 28, 2023

Uh oh!

joshuaobrien Dec 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexr00 Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

joshuaobrien Dec 3, 2023

Choose a reason for hiding this comment

Uh oh!

alexr00 Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexr00 left a comment

Choose a reason for hiding this comment

Uh oh!

alexr00 Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

alexr00 Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

alexr00 Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshuaobrien commented Dec 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshuaobrien commented Apr 3, 2023 •

edited

Loading

joshuaobrien Dec 3, 2023 •

edited

Loading