-
Notifications
You must be signed in to change notification settings - Fork 230
Description
Problem
When an API revision is deleted (soft or hard), its associated comments remain in the database with their original APIRevisionId unchanged. This creates orphaned comments that reference revisions that no longer exist. The frontend works around this by falling back orphaned comments to the active revision for display, but the underlying data relationship is broken.
There are two distinct scenarios where this matters:
Scenario 1: Revision Replacement (Automatic Revisions)
When AutoReviewService.CreateAutomaticRevisionAsync creates a new automatic revision, it soft-deletes superseded revisions that are not approved, not released, and not identical to the incoming revision. The check in AutoReviewService.cs protects revisions that have comments:
comments.Any(c => latestAutomaticAPIRevision.Id == c.APIRevisionId)However, CarryForwardRevisionDataAsync only carries forward approval status and the HasAutoGeneratedComments flag — it does not re-associate comment records to the new revision. This means:
- If a revision has no comments, it gets deleted and replaced — no issue yet.
- If a revision does have comments, it is kept around solely to preserve the comment linkage, even though it has been logically superseded. Over time this leads to stale revisions that exist only as comment anchors.
- The comments'
APIRevisionIdis never updated to point to the successor revision.
Scenario 2: Direct Deletion (Archive, PR Cleanup, Manual Delete, Purge)
When revisions are deleted via:
- Auto-archive — inactive manual revisions older than N months
- PR cleanup — closed PR revisions with no approvers
- Manual user deletion
- Auto-purge — hard delete of long-soft-deleted revisions (Cosmos DB entry + blob storage removed)
...no cleanup or migration of associated comments occurs. The comments remain in the Comments Cosmos container referencing a revision ID that may no longer exist in the Revisions container.
Expected Behavior
-
Replacement operations (automatic revision superseding): All non-diagnostic user comments from the old revision should be re-associated to the replacing revision (update
comment.APIRevisionIdto the new revision ID). -
Direct deletion (no replacement): Comments associated with the deleted revision should be deleted (or soft-deleted) along with the revision.
-
Diagnostic comments should always be deleted with their revision, since they are revision-specific by nature.
Current Workarounds in the Frontend
The Conversations panel in conversations.component.ts has a fallback that maps orphaned comments to the active revision for display:
// If the thread's apiRevisionId doesn't match any loaded revision,
// fall back to the active revision so it still appears in the panel.
if (apiRevisionPostion === Number.MAX_SAFE_INTEGER && this.activeApiRevisionId) {
const activePosition = apiRevisionPositionMap.get(this.activeApiRevisionId);
if (activePosition !== undefined) {
apiRevisionPostion = activePosition;
}
}The comment-visibility.helper.ts also shows all non-diagnostic comments regardless of revision, which further hides orphaned comments from users. These workarounds mask the issue at the UI level but do not fix the data integrity problem.
Affected Code Paths
| Path | File | Action Needed |
|---|---|---|
| Automatic revision creation | AutoReviewService.cs |
Re-associate comments to new revision before soft-deleting old one |
CarryForwardRevisionDataAsync |
APIRevisionsManager.cs |
Extend to migrate comment APIRevisionId references |
SoftDeleteAPIRevisionAsync |
APIRevisionsManager.cs |
Soft-delete associated comments (or migrate them if a replacement exists) |
AutoPurgeAPIRevisions |
APIRevisionsManager.cs |
Delete orphaned comments when hard-purging revisions |
ClosePullRequestAPIRevision |
PullRequestManager.cs |
Handle associated comments on PR revision cleanup |
AutoArchiveAPIRevisions |
APIRevisionsManager.cs |
Handle associated comments on archive |
Metadata
Metadata
Assignees
Labels
Type
Projects
Status