SCM - revert back to computing incoming/outgoing changes using the history item view models#281273
SCM - revert back to computing incoming/outgoing changes using the history item view models#281273
Conversation
…story item view models
There was a problem hiding this comment.
Pull request overview
This PR reverts the logic for computing incoming/outgoing changes in the SCM history graph back to using view models instead of raw history items. This change fixes issue #280265 by performing incoming/outgoing node insertion after view models are created, allowing the code to leverage swimlane colors for proper graph rendering.
Key Changes:
- Moved the computation of incoming/outgoing changes from operating on
ISCMHistoryItem[]toISCMHistoryItemViewModel[] - Removed the
getHistoryItemViewModelKindfunction as the kind is now determined inline during view model creation - Updated
addIncomingOutgoingChangesHistoryItemsto work with view models and manipulate swimlane connections - Skipped a test case that likely needs updates to align with the new implementation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/scm/browser/scmHistory.ts | Refactored incoming/outgoing changes computation to work with view models post-creation, enabling proper swimlane color usage and graph node insertion |
| src/vs/workbench/contrib/scm/test/browser/scmHistory.test.ts | Skipped test case "graph with incoming/outgoing changes (remote ref first)" that may need updates for the new implementation |
| viewModels[afterHistoryItemIndex].inputSwimlanes.push({ | ||
| id: currentHistoryItemRef.revision, | ||
| color: historyItemRefColor | ||
| }); |
There was a problem hiding this comment.
Direct mutation of viewModels[afterHistoryItemIndex].inputSwimlanes by pushing a new node violates immutability principles and is inconsistent with the pattern used for incoming changes (lines 445-459), where a new view model object is created via the spread operator.
This should follow the same pattern as incoming changes by creating a new view model object:
viewModels[afterHistoryItemIndex] = {
...viewModels[afterHistoryItemIndex],
inputSwimlanes: [...viewModels[afterHistoryItemIndex].inputSwimlanes, {
id: currentHistoryItemRef.revision,
color: historyItemRefColor
}]
};| viewModels[afterHistoryItemIndex].inputSwimlanes.push({ | |
| id: currentHistoryItemRef.revision, | |
| color: historyItemRefColor | |
| }); | |
| viewModels[afterHistoryItemIndex] = { | |
| ...viewModels[afterHistoryItemIndex], | |
| inputSwimlanes: [ | |
| ...viewModels[afterHistoryItemIndex].inputSwimlanes, | |
| { | |
| id: currentHistoryItemRef.revision, | |
| color: historyItemRefColor | |
| } | |
| ] | |
| }; |
| * * f(g) | ||
| */ | ||
| test('graph with incoming/outgoing changes (remote ref first)', () => { | ||
| test.skip('graph with incoming/outgoing changes (remote ref first)', () => { |
There was a problem hiding this comment.
The test "graph with incoming/outgoing changes (remote ref first)" is being skipped, but there's no explanation or tracking issue comment indicating why it's skipped or when it should be re-enabled.
According to the PR description (Fixes #280265), this PR is supposed to fix an issue. If this test is expected to pass after the fix, it should be unskipped. If it needs to remain skipped, add a comment explaining why (e.g., // TODO: Re-enable once #XXXXX is fixed).
…story item view models (microsoft#281273) <!-- Thank you for submitting a Pull Request. Please: * Read our Pull Request guidelines: https://github.com/microsoft/vscode/wiki/How-to-Contribute#pull-requests * Associate an issue with the Pull Request. * Ensure that the code is up-to-date with the `main` branch. * Include a description of the proposed changes and how to test them. -->
Fixes #280265