Add `shouldStick` to IStickyScrollDelegate for non-parent sticky nodes #274639

Copilot · 2025-11-02T22:05:00Z

Plan to add sticky scroll support for SCM input node

Summary

Successfully implemented sticky scroll support for non-parent nodes in the tree component. The SCM input field and action buttons will now remain visible when scrolling through many file changes in the Source Control view.

The implementation properly handles compressed nodes by unwrapping them through the compression model and checking each element. The type assertion makes it clear that we're creating ITreeNode<T, TFilterData> instances from the compressed structure by replacing the element field.

Original prompt

This section details on the original issue you should resolve

<issue_title>SCM - Make SCM button and input in workbench sticky</issue_title>
<issue_description>

When there are many changes, having sticky SCM actions would make it easier to commit after staging files without needing to scroll back to the top.

https://github.com/user-attachments/assets/e8dbf041-39bf-4db1-87a9-82b66dd15341</issue_description>

<agent_instructions>Add a method to the IStickyScrollDelegate called shouldStick which when passed with a node returns a boolean whether the node should stick even though it is not a parent node. Such a node which is not a parent can stick if shouldStick returns true and the parent node is either sticky or the root, and scroll top of the tree is larger than the node top position. This should be computed in the getNextStickyNode method and check all children of previousStickyNode and then compare with nextStickyNode and see which comes first in the order. If the node with shouldStick true is first then it should be returned as a sticky node, but the next call to getNextStickyNode should use the same previous sticky node as before. The SCM tree view should create a sticky scroll delegate which has shouldStick returning true for the scm input node</agent_instructions>

Comments on the Issue (you are @copilot in this section)
@lszomoru @benibenj, could we add support for the tree consumer to specify that some nodes are sticky?

Fixes SCM - Make SCM button and input in workbench sticky #274524

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

… nodes Co-authored-by: benibenj <[email protected]>

Co-authored-by: benibenj <[email protected]>

lszomoru · 2025-11-03T06:23:17Z

@copilot, one potential issue here is that the CompressibleObjectTree which is being used in the Changes view is always using the CompressibleStickyScrollDelegate and it does not take into account the scroll delegate from options. This would have to be adjusted as well.

Co-authored-by: lszomoru <[email protected]>

Copilot · 2025-11-03T06:45:08Z

@copilot, one potential issue here is that the CompressibleObjectTree which is being used in the Changes view is always using the CompressibleStickyScrollDelegate and it does not take into account...

Fixed in commit 1dde16f.

The CompressibleStickyScrollDelegate now accepts an optional base delegate in its constructor and delegates shouldStick calls to it. The CompressibleObjectTree constructor passes options.stickyScrollDelegate to CompressibleStickyScrollDelegate, ensuring the custom delegate's shouldStick logic is respected while maintaining compression functionality.

lszomoru · 2025-11-03T19:09:37Z

@copilot, I have checked out the branch but with these changes the SCMInput does not seem to be "sticking" to the top of the tree as required. Something seems to be missing.

Co-authored-by: lszomoru <[email protected]>

Copilot · 2025-11-03T19:15:13Z

@copilot, I have checked out the branch but with these changes the SCMInput does not seem to be "sticking" to the top of the tree as required. Something seems to be missing.

Fixed in commit f3ae284.

The issue was that the tree options were missing enableStickyScroll: true. The sticky scroll controller is only created when this option is explicitly enabled. I've added this option to the SCM tree configuration, which will now activate the sticky scroll functionality with the custom SCMStickyScrollDelegate.

benibenj · 2025-11-03T22:45:57Z

src/vs/workbench/contrib/scm/browser/scmViewPane.ts

@@ -2417,7 +2422,9 @@ export class SCMViewPane extends ViewPane {
 					// History Item Group, History Item, or History Item Change
 					return (viewState?.expanded ?? []).indexOf(getSCMResourceId(e as TreeElement)) === -1;
 				},
-				accessibilityProvider: this.instantiationService.createInstance(SCMAccessibilityProvider)
+				accessibilityProvider: this.instantiationService.createInstance(SCMAccessibilityProvider),
+				enableStickyScroll: true,


@copilot enableStickyScroll: true, is not needed

Fixed in commit adf87f8. Removed enableStickyScroll: true from the tree options.

benibenj · 2025-11-03T22:49:55Z

src/vs/base/browser/ui/tree/objectTree.ts

+
+	shouldStick(node: ITreeNode<T, TFilterData>): boolean {
+		// Delegate to the base delegate if provided
+		return this.baseDelegate?.shouldStick?.(node) ?? false;


@copilot The node is actually a compressed node which needs to be unwrapped. Compare with other code which works with compressed nodes. Something in the following direction might be possible.

const compressionModel = this.modelProvider(); const compressedNode = compressionModel.getCompressedTreeNode(node.element); const fistNode = compressedNode.element?.elements[0];

I think some types are a bit off. Don't forget this is a compressed tree

Fixed in commit adf87f8. The shouldStick method now unwraps compressed nodes using compressionModel.getCompressedTreeNode() and checks each element in the compressed node's elements array.

benibenj · 2025-11-03T22:53:32Z

src/vs/base/browser/ui/tree/abstractTree.ts

+		// Check all direct children of the parent node
+		for (const childNode of parentNode.children) {
+			// Check if this child should stick
+			if (!this.stickyScrollDelegate.shouldStick(childNode)) {


@copilot This will never make progress if a node returns true for shouldStick as next time it will be called the same node will be checked again and will return true again. This will run infinitely. It should remember which nodes it checked and start looking from that node onwards. Be a bit smarter regarding this but make sure to not make the code look more ugly, it should be nicely readable without to many comments

Fixed in commit adf87f8. The infinite loop is now prevented by tracking processed shouldStick nodes in a Set. When a shouldStick node is found and added to sticky nodes, it's added to processedShouldStickNodes and skipped in subsequent iterations, allowing the algorithm to progress through all children without getting stuck.

…d nodes, prevent infinite loop Co-authored-by: benibenj <[email protected]>

benibenj · 2025-11-04T08:44:31Z

src/vs/base/browser/ui/tree/objectTree.ts

+			for (const element of compressedNode.element.elements) {
+				// Create a temporary node with the unwrapped element to check
+				const unwrappedNode: ITreeNode<T, TFilterData> = { ...node, element };
+				if (this.baseDelegate.shouldStick(unwrappedNode)) {


@copilot It should be this.baseDelegate.shouldStick(unwrappedNode.element). but you should not use a temporary node. Something seems off with the types. Look at the big picture. Don't forget a node from SCM tree is wrapped into a compressed node in the CompressibleObjectTree which is then given to the abstract tree. To the abstract tree, the compressed node looks like a regular node.

Maybe it should be shouldStick(node: ITreeNode<ICompressedTreeNode<T> | null, TFilterData>): boolean { but then you'll need to look at the types of CompressibleStickyScrollDelegate, baseDelegate and IStickyScrollDelegate and see what's wrong.

@copilot do this

@copilot work on my comment above

Fixed in commit b6ccf41. The shouldStick method now properly unwraps compressed nodes by getting the compressed node structure from the model and creating unwrapped nodes with { ...compressedNode, element } for each element in the compressed array. This avoids the temporary node issue and correctly passes ITreeNode<T, TFilterData> to the base delegate.

Copilot · 2025-11-04T08:44:38Z

@benibenj Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: b2c65750-f893-4008-ac32-b2429837583f

Sorry for the inconvenience!

Copilot · 2025-11-04T08:45:16Z

@benibenj Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 9c77cee4-cdcb-42b2-b93e-c06cccf52db1

Sorry for the inconvenience!

… node Co-authored-by: benibenj <[email protected]>

benibenj · 2025-11-04T11:32:28Z

src/vs/base/browser/ui/tree/objectTree.ts

+		if (compressedNode.element) {
+			for (const element of compressedNode.element.elements) {
+				// Create a node with the unwrapped element for the base delegate
+				if (this.baseDelegate.shouldStick({ ...compressedNode, element })) {


@copilot t should be passing the element to shouldStick but something seems off with the types of the method. Look at the big picture. Don't forget a node from SCM tree is wrapped into a compressed node in the CompressibleObjectTree which is then given to the abstract tree. To the abstract tree, the compressed node looks like a regular node.

Maybe it should be shouldStick(node: ITreeNode<ICompressedTreeNode | null, TFilterData>): boolean { but then you'll need to look at the types of CompressibleStickyScrollDelegate, baseDelegate and IStickyScrollDelegate and see what's wrong.

Fixed in commit d0d7cbc. Added an explicit type assertion as ITreeNode<T, TFilterData> to make it clear that we're creating a node with the unwrapped element type for the base delegate. The spread operator { ...compressedNode, element } takes the structure from the compressed node and replaces the element field with the unwrapped element of type T.

Co-authored-by: benibenj <[email protected]>

Initial plan

f57aec8

Copilot AI assigned Copilot and benibenj Nov 2, 2025

Copilot started work on behalf of benibenj November 2, 2025 22:05 View session

Copilot AI and others added 4 commits November 2, 2025 22:23

Add shouldStick method to IStickyScrollDelegate for sticky non-parent…

4e47945

… nodes Co-authored-by: benibenj <[email protected]>

Refine shouldStick node logic to check scroll position correctly

5045be5

Co-authored-by: benibenj <[email protected]>

Improve shouldStick logic to find first candidate node

d615663

Co-authored-by: benibenj <[email protected]>

Address code review feedback - extend DefaultStickyScrollDelegate

5727fd8

Co-authored-by: benibenj <[email protected]>

Copilot AI changed the title ~~[WIP] Add sticky functionality to SCM button and input in workbench~~ Add shouldStick to IStickyScrollDelegate for non-parent sticky nodes Nov 2, 2025

Copilot AI requested a review from benibenj November 2, 2025 22:37

Copilot finished work on behalf of benibenj November 2, 2025 22:37

Copilot started work on behalf of lszomoru November 3, 2025 06:23 View session

Copilot AI and others added 2 commits November 3, 2025 06:37

Fix CompressibleObjectTree to respect stickyScrollDelegate from options

de827c8

Co-authored-by: lszomoru <[email protected]>

Fix compilation errors - remove override and unused imports

1dde16f

Co-authored-by: lszomoru <[email protected]>

Copilot AI requested a review from lszomoru November 3, 2025 06:46

Copilot finished work on behalf of lszomoru November 3, 2025 06:46

Copilot started work on behalf of lszomoru November 3, 2025 19:10 View session

Enable sticky scroll for SCM tree by setting enableStickyScroll: true

f3ae284

Co-authored-by: lszomoru <[email protected]>

Copilot finished work on behalf of lszomoru November 3, 2025 19:16

benibenj requested changes Nov 3, 2025

View reviewed changes

Copilot started work on behalf of benibenj November 3, 2025 22:54 View session

Fix sticky scroll issues: remove enableStickyScroll, unwrap compresse…

adf87f8

…d nodes, prevent infinite loop Co-authored-by: benibenj <[email protected]>

Copilot AI requested a review from benibenj November 3, 2025 23:00

Copilot finished work on behalf of benibenj November 3, 2025 23:00

benibenj reviewed Nov 4, 2025

View reviewed changes

Copilot started work on behalf of benibenj November 4, 2025 11:20 View session

Fix shouldStick to properly unwrap compressed nodes without temporary…

b6ccf41

… node Co-authored-by: benibenj <[email protected]>

Copilot stopped work on behalf of benibenj due to an error November 4, 2025 11:27
An unexpected error occurred. For more details, see the detailed logs in GitHub Actions.

Copilot AI requested a review from benibenj November 4, 2025 11:27

benibenj reviewed Nov 4, 2025

View reviewed changes

Copilot started work on behalf of benibenj November 4, 2025 11:33 View session

Add explicit type assertion for unwrapped node passed to base delegate

d0d7cbc

Co-authored-by: benibenj <[email protected]>

Copilot AI requested a review from benibenj November 4, 2025 11:38

Copilot finished work on behalf of benibenj November 4, 2025 11:38

benibenj mentioned this pull request Nov 4, 2025

Exclusively use emojis for comment reactions (was Consider allowing codicons in comment reactions) #126995

Closed

Add shouldStick to IStickyScrollDelegate for non-parent sticky nodes #274639

Are you sure you want to change the base?

Add shouldStick to IStickyScrollDelegate for non-parent sticky nodes #274639

Conversation

Copilot AI commented Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Plan to add sticky scroll support for SCM input node

Summary

Comments on the Issue (you are @copilot in this section)

Uh oh!

lszomoru commented Nov 3, 2025

Uh oh!

Copilot AI commented Nov 3, 2025

Uh oh!

lszomoru commented Nov 3, 2025

Uh oh!

Copilot AI commented Nov 3, 2025

Uh oh!

benibenj Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

benibenj Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

benibenj Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

benibenj Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

benibenj Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

benibenj Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Nov 4, 2025

Uh oh!

Copilot AI commented Nov 4, 2025

Uh oh!

benibenj Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `shouldStick` to IStickyScrollDelegate for non-parent sticky nodes #274639

Add `shouldStick` to IStickyScrollDelegate for non-parent sticky nodes #274639

Copilot AI commented Nov 2, 2025 •

edited

Loading