SCM - refactor incoming/outgoing changes insertion#277347
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the logic for inserting incoming and outgoing changes history items in the SCM view. The main change moves from post-processing view models to directly inserting synthetic history items into the history items array before view model generation.
Key changes:
- Refactored incoming/outgoing changes insertion to modify the history items array directly instead of post-processing view models
- Simplified the logic by resolving the common ancestor (merge base) using ref names instead of IDs
- Added edge case handling for already-merged incoming changes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/scm/browser/scmHistoryViewPane.ts | Updated action handler to use ref names for common ancestor resolution; improved null checks; updated comments for clarity |
| src/vs/workbench/contrib/scm/browser/scmHistory.ts | Removed unused import; refactored incoming/outgoing changes insertion into a new function that modifies the history items array directly; added label color logic for synthetic history items |
| const incomingChangeMerged = historyItems[beforeHistoryItemIndex].parentIds.length === 2 && | ||
| historyItems[beforeHistoryItemIndex].parentIds.includes(mergeBase); | ||
|
|
||
| if (beforeHistoryItemIndex !== -1 && afterHistoryItemIndex !== -1 && !incomingChangeMerged) { | ||
| // Insert incoming history item | ||
| historyItems.splice(afterHistoryItemIndex, 0, { |
There was a problem hiding this comment.
Potential array access with invalid index. The code checks historyItems[beforeHistoryItemIndex].parentIds on line 457 before verifying that beforeHistoryItemIndex !== -1 on line 460. If beforeHistoryItemIndex is -1 (not found), this will throw an error when trying to access the parentIds property of undefined.
The check on line 460 should be moved before line 457, or the logic on line 457 should include a guard condition for beforeHistoryItemIndex !== -1.
| const incomingChangeMerged = historyItems[beforeHistoryItemIndex].parentIds.length === 2 && | |
| historyItems[beforeHistoryItemIndex].parentIds.includes(mergeBase); | |
| if (beforeHistoryItemIndex !== -1 && afterHistoryItemIndex !== -1 && !incomingChangeMerged) { | |
| // Insert incoming history item | |
| historyItems.splice(afterHistoryItemIndex, 0, { | |
| if (beforeHistoryItemIndex !== -1 && afterHistoryItemIndex !== -1) { | |
| const incomingChangeMerged = historyItems[beforeHistoryItemIndex].parentIds.length === 2 && | |
| historyItems[beforeHistoryItemIndex].parentIds.includes(mergeBase); | |
| if (!incomingChangeMerged) { | |
| // Insert incoming history item | |
| historyItems.splice(afterHistoryItemIndex, 0, { |
| for (let index = currentHistoryItemRemoteIndex; index < historyItems.length; index++) { | ||
| if (historyItems[index].parentIds.includes(mergeBase)) { | ||
| beforeHistoryItemIndex = index; | ||
| break; | ||
| } | ||
|
|
||
| if (historyItems[index].parentIds.includes(historyItemParentId)) { | ||
| historyItemParentId = historyItems[index].parentIds[0]; |
There was a problem hiding this comment.
Potential undefined access. If currentHistoryItemRemoteIndex is -1 (when the history item is not found), or if the found history item has no parent IDs, historyItemParentId will be undefined on line 439. This undefined value is then used in line 446 within parentIds.includes(historyItemParentId), which could lead to unexpected behavior.
Consider adding a guard to check if currentHistoryItemRemoteIndex !== -1 and if historyItemParentId is defined before the loop, or early-return if these conditions aren't met.
| for (let index = currentHistoryItemRemoteIndex; index < historyItems.length; index++) { | |
| if (historyItems[index].parentIds.includes(mergeBase)) { | |
| beforeHistoryItemIndex = index; | |
| break; | |
| } | |
| if (historyItems[index].parentIds.includes(historyItemParentId)) { | |
| historyItemParentId = historyItems[index].parentIds[0]; | |
| if (currentHistoryItemRemoteIndex !== -1 && typeof historyItemParentId !== 'undefined') { | |
| for (let index = currentHistoryItemRemoteIndex; index < historyItems.length; index++) { | |
| if (historyItems[index].parentIds.includes(mergeBase)) { | |
| beforeHistoryItemIndex = index; | |
| break; | |
| } | |
| if (historyItems[index].parentIds.includes(historyItemParentId)) { | |
| historyItemParentId = historyItems[index].parentIds[0]; | |
| } |
| id: SCMIncomingHistoryItemId, | ||
| displayId: '0'.repeat(displayIdLength), | ||
| parentIds: [mergeBase], | ||
| displayId: '0'.repeat(historyItems[0].displayId?.length ?? 0), |
There was a problem hiding this comment.
Potential array access on empty array. The code accesses historyItems[0].displayId?.length without verifying that historyItems is not empty. If the function is called with an empty historyItems array, this will throw an error when trying to access index 0 of an empty array.
Consider adding a guard to check historyItems.length > 0 at the start of the function, or use a fallback value like historyItems[0]?.displayId?.length ?? 0.
| displayId: '0'.repeat(historyItems[0].displayId?.length ?? 0), | |
| displayId: '0'.repeat(historyItems[0]?.displayId?.length ?? 0), |
| id: SCMOutgoingHistoryItemId, | ||
| displayId: '0'.repeat(displayIdLength), | ||
| parentIds: [mergeBase], | ||
| displayId: '0'.repeat(historyItems[0].displayId?.length ?? 0), |
There was a problem hiding this comment.
Potential array access on empty array. The code accesses historyItems[0].displayId?.length without verifying that historyItems is not empty. If the function is called with an empty historyItems array, this will throw an error when trying to access index 0 of an empty array.
Consider adding a guard to check historyItems.length > 0 at the start of the function, or use a fallback value like historyItems[0]?.displayId?.length ?? 0.
Fixes #277239
Fixes #276064