Conversation
375cfae to
6189c3d
Compare
590bba8 to
cbe3261
Compare
| [JsonProperty("description")] | ||
| public string Description { get; } | ||
|
|
||
| [JsonProperty("sourceRepoFlowSha")] |
There was a problem hiding this comment.
Rename to "codeflowSourceSha" ?
|
Marking this as ready to get the PR summary from copilot |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new API endpoint for retrieving codeflow history between repositories and the VMR (Virtual Mono Repository), including a visualization component in the BarViz UI. The implementation adds Redis-based caching for commit history and integrates GitHub API calls to fetch blame and commit data for tracking forward flows and backflows.
Changes:
- Adds
/api/subscriptions/{id}/codeflowhistoryendpoint to fetch and visualize code flow between repositories - Implements
CodeflowHistoryManagerwith Redis caching for commit history and codeflow metadata - Extends DarcLib with new methods for fetching commits and determining last incoming flows via GitHub GraphQL and REST APIs
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 35 comments.
Show a summary per file
| File | Description |
|---|---|
| PullRequestUpdater.cs | Adds placeholder TODO comments and unused Kusto.Ingest import for future codeflow synchronization |
| CodeflowHistoryManager.cs | New service implementing Redis-based caching and GitHub API integration for codeflow history tracking |
| CodeflowHistoryResult.cs | API response model for codeflow history data |
| SubscriptionsController.cs (v2018_07_16) | Adds base implementation of GetCodeflowHistory endpoint with logic to fetch and format commit graphs |
| SubscriptionsController.cs (v2019_01_16) | Adds commented-out endpoint stub for this API version |
| SubscriptionsController.cs (v2020_02_20) | Adds public endpoint definition delegating to base implementation |
| SubscriptionDetailDialog.razor | Replaces subscription details table with codeflow history graph visualization |
| CodeflowHistoryGraph.razor | New SVG-based visualization component for rendering commit history with flow arrows |
| PcsStartup.cs | Registers CodeflowHistoryManager as scoped service |
| Subscriptions.cs (Generated Client) | Adds generated client method for calling the new API endpoint |
| CodeflowHistory.cs (Generated Models) | Generated client models for API responses |
| IRemote.cs, IRemoteGitRepo.cs | Extends interfaces with methods for fetching commits and determining last flows |
| Remote.cs | Wrapper methods delegating to RemoteGitClient implementations |
| GitHubClient.cs | Implements commit fetching, blame API calls, and codeflow detection via source manifests and version details |
| AzureDevOpsClient.cs | Stub implementations throwing NotImplementedException |
| GitRepoFactory.cs, RemoteFactory.cs (multiple) | Updates to pass IVersionDetailsParser dependency to GitHubClient |
| appsettings.Development.json | Formatting-only change to comment alignment |
| ConfigurationOptions options) : ICodeflowHistoryManager | ||
| { | ||
| private readonly IRemoteFactory _remoteFactory = remoteFactory; | ||
| private readonly IConnectionMultiplexer _connection = ConnectionMultiplexer.Connect(options); |
There was a problem hiding this comment.
The IConnectionMultiplexer is created in the constructor but is never disposed. This class should implement IDisposable or IAsyncDisposable to properly dispose of the Redis connection, or the connection should be managed as a singleton with a different lifetime strategy.
| if (commitValues.Any(val => !val.HasValue)) | ||
| { | ||
| await ClearCodeflowCacheAsync(subscriptionId); | ||
| throw new InvalidOperationException($"Corrupted commit data encountered for subscription `{subscriptionId}`."); |
There was a problem hiding this comment.
The cache is cleared when corrupted commit data is encountered, but the exception thrown immediately after means the cache clear operation may not persist if there's a transaction or if the method caller doesn't handle this properly. Consider logging this condition and whether the cache clear should happen before or after throwing.
| using Maestro.Data.Models; | ||
| using Microsoft.DotNet.DarcLib; | ||
| using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; | ||
| using Pipelines.Sockets.Unofficial.Arenas; |
There was a problem hiding this comment.
The unused import Pipelines.Sockets.Unofficial.Arenas should be removed as it's not used anywhere in this file.
| using Pipelines.Sockets.Unofficial.Arenas; |
...tConstructionService/ProductConstructionService.BarViz/Components/CodeflowHistoryGraph.razor
Show resolved
Hide resolved
| .Where(sub => | ||
| sub.SourceRepository == subscription.TargetRepository | ||
| && sub.TargetRepository == subscription.SourceRepository) | ||
| .FirstOrDefaultAsync(sub => sub.SourceEnabled == true); |
There was a problem hiding this comment.
The expression 'A == true' can be simplified to 'A'.
| { | ||
| var subscription = await _context.Subscriptions | ||
| .Include(sub => sub.LastAppliedBuild) | ||
| .FirstOrDefaultAsync(sub => sub.Id == id && sub.SourceEnabled == true); |
There was a problem hiding this comment.
The expression 'A == true' can be simplified to 'A'.
| new("b3n5n7", "Grace", "Remove unused code", null), | ||
| }; | ||
|
|
||
| private List<CodeflowGraphCommit> _vmrCommits = new List<CodeflowGraphCommit> |
There was a problem hiding this comment.
Field '_vmrCommits' can be 'readonly'.
| private List<CodeflowGraphCommit> _vmrCommits = new List<CodeflowGraphCommit> | |
| private readonly List<CodeflowGraphCommit> _vmrCommits = new List<CodeflowGraphCommit> |
| return result; | ||
| } | ||
|
|
||
| private List<CodeflowGraphCommit> _repoCommits = new List<CodeflowGraphCommit> |
There was a problem hiding this comment.
Field '_repoCommits' can be 'readonly'.
| private List<CodeflowGraphCommit> _repoCommits = new List<CodeflowGraphCommit> | |
| private readonly List<CodeflowGraphCommit> _repoCommits = new List<CodeflowGraphCommit> |
| @((MarkupString)$"<text x=\"0\" y=\"-10\" fill=\"#000\">{_leftColumnName}</text>") | ||
| <line x1="60" y1="0" x2="60" y2="@((_leftColumn.Count - 1) * (BOX_HEIGHT + BOX_Y_MARGIN))" stroke="#bbb" stroke-width="1.5" /> | ||
| @for (int i = 0; i < _leftColumn.Count; i++) | ||
| { | ||
| if (!_leftColumn[i].Contains("hidden")) | ||
| { | ||
| <use href="#commitBox" x="0" y="@(i * (BOX_HEIGHT + BOX_Y_MARGIN))" /> | ||
| @((MarkupString)$"<text x=\"{BOX_WIDTH / 2}\" y=\"{i * (BOX_HEIGHT + BOX_Y_MARGIN) + 32}\" text-anchor=\"middle\" fill=\"#000\">{_leftColumn[i]}</text>") | ||
| ; | ||
| } | ||
| else | ||
| { | ||
|
|
||
| <use href="#whiteBox" x="-20" y="@(i * (BOX_HEIGHT + BOX_Y_MARGIN))" /> | ||
| var firstLine = _leftColumn[i].Split('(')[0]; | ||
| var secondLine = "(" + _leftColumn[i].Split('(')[1]; | ||
| @((MarkupString)$"<text x=\"{BOX_WIDTH / 2}\" y=\"{i * (BOX_HEIGHT + BOX_Y_MARGIN) + 22}\" text-anchor=\"middle\" fill=\"#000\">{firstLine}</text>") | ||
| ; | ||
| @((MarkupString)$"<text x=\"{BOX_WIDTH / 2}\" y=\"{i * (BOX_HEIGHT + BOX_Y_MARGIN) + 42}\" text-anchor=\"middle\" fill=\"#000\">{secondLine}</text>") | ||
| ; | ||
| } | ||
| } | ||
| </g> | ||
|
|
||
| <!-- RIGHT COLUMN --> | ||
| <g class="column right" transform="translate(@(COL_SPACE), 50)"> | ||
| @((MarkupString)$"<text x=\"0\" y=\"-10\" fill=\"#000\">{_rightColumnName}</text>") | ||
| <line x1="60" y1="0" x2="60" y2="@((_rightColumn.Count - 1) * (BOX_HEIGHT + BOX_Y_MARGIN))" stroke="#bbb" stroke-width="1.5" /> | ||
| @for (int i = 0; i < _rightColumn.Count; i++) | ||
| { | ||
| if (!_rightColumn[i].Contains("hidden")) | ||
| { | ||
| <use href="#commitBox" x="0" y="@(i * (BOX_HEIGHT + BOX_Y_MARGIN))" /> | ||
| @((MarkupString)$"<text x=\"{BOX_WIDTH / 2}\" y=\"{i * (BOX_HEIGHT + BOX_Y_MARGIN) + 32}\" text-anchor=\"middle\" fill=\"#000\">{_rightColumn[i]}</text>") | ||
| ; |
There was a problem hiding this comment.
This component uses MarkupString to inject _leftColumnName, _rightColumnName, and commit IDs directly into SVG <text> elements without HTML encoding, while those values are populated from the CodeflowHistory API (e.g., RepoName, which ultimately comes from subscription configuration). An attacker who can control subscription data (such as SourceDirectory or TargetBranch) could supply values containing HTML like </text><script>...</script> and achieve stored XSS against any user viewing this graph. Replace MarkupString usage with standard Blazor text rendering or explicitly HTML-encode/sanitize all dynamic values before constructing markup so that user-controlled data cannot break out of the intended <text> context.
No description provided.