SCM - remove the usage of querySelector#276766
Conversation
This reverts commit 4d84cf6.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SCM history view to remove the usage of querySelector for accessing twistie elements, replacing it with a simpler and more direct DOM navigation approach using previousElementSibling. This makes the code more maintainable and follows better practices by avoiding deep DOM traversal.
Key Changes:
- Replaced
querySelectorcalls withpreviousElementSiblingto access twistie elements in two renderers - Updated CSS selector to be more specific by requiring the
.force-no-twistieclass
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/scm/browser/scmHistoryViewPane.ts |
Replaced querySelector with previousElementSibling in HistoryItemRenderer and HistoryItemLoadMoreRenderer |
src/vs/workbench/contrib/scm/browser/media/scm.css |
Made the twistie hiding CSS selector more specific by requiring the .force-no-twistie class |
| // HACK - add .force-no-twistie class to the twistie element | ||
| container.previousElementSibling?.classList.add('force-no-twistie'); |
There was a problem hiding this comment.
The approach using previousElementSibling assumes a specific DOM structure that may be fragile. If the tree implementation changes and adds another sibling element before the twistie (e.g., between indent and twistie), this code will silently fail or add the class to the wrong element. Consider either: 1) Adding a runtime check to verify the element has the 'monaco-tl-twistie' class before adding 'force-no-twistie', or 2) Documenting this dependency on the tree's internal DOM structure with a reference to abstractTree.ts lines 409-414.
| // HACK - add .force-no-twistie class to the twistie element | |
| container.previousElementSibling?.classList.add('force-no-twistie'); | |
| // HACK: Add .force-no-twistie class to the twistie element if present. | |
| // This depends on the tree's internal DOM structure: see abstractTree.ts lines 409-414. | |
| const prev = container.previousElementSibling; | |
| if (prev && prev.classList.contains('monaco-tl-twistie')) { | |
| prev.classList.add('force-no-twistie'); | |
| } |
|
|
||
| renderTemplate(container: HTMLElement): LoadMoreTemplate { | ||
| // HACK - add .force-no-twistie class to the twistie element | ||
| container.previousElementSibling?.classList.add('force-no-twistie'); |
There was a problem hiding this comment.
The approach using previousElementSibling assumes a specific DOM structure that may be fragile. If the tree implementation changes and adds another sibling element before the twistie (e.g., between indent and twistie), this code will silently fail or add the class to the wrong element. Consider either: 1) Adding a runtime check to verify the element has the 'monaco-tl-twistie' class before adding 'force-no-twistie', or 2) Documenting this dependency on the tree's internal DOM structure with a reference to abstractTree.ts lines 409-414.
| container.previousElementSibling?.classList.add('force-no-twistie'); | |
| const prev = container.previousElementSibling; | |
| // Defensive: Only add if previous sibling is the twistie. See abstractTree.ts lines 409-414 for structure. | |
| if (prev && prev.classList.contains('monaco-tl-twistie')) { | |
| prev.classList.add('force-no-twistie'); | |
| } |
No description provided.