Skip to content

feat(preprod): Add snapshot PR comment templates#112353

Merged
runningcode merged 5 commits intomasterfrom
no/snapshot-pr-comment-templates
Apr 9, 2026
Merged

feat(preprod): Add snapshot PR comment templates#112353
runningcode merged 5 commits intomasterfrom
no/snapshot-pr-comment-templates

Conversation

@runningcode
Copy link
Copy Markdown
Contributor

@runningcode runningcode commented Apr 7, 2026

Add formatting for snapshot PR comments that will be posted to GitHub
PRs when snapshot comparisons complete. Single function handles all
cases — comparison results, no-base uploads, processing, and failures —
using the same table format throughout, consistent with how size
analysis works.

This is the first of three PRs to implement snapshot PR comments
(EME-999). Templates only — no runtime effect. The Celery task and
trigger wiring will follow in subsequent PRs.

Refs EME-999

Add formatting functions for snapshot PR comments that will be posted
to GitHub PRs when snapshot comparisons complete. Two functions:

- format_snapshot_pr_comment: comparison table with linked counts for
  added/removed/modified/renamed/unchanged and approval status
- format_snapshot_pr_comment_solo: solo upload table for first uploads
  and missing base scenarios

This is the first of three PRs — templates only, no runtime effect.
The task and wiring will follow.
@linear-code
Copy link
Copy Markdown

linear-code bot commented Apr 7, 2026

EME-999 PR comments

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 7, 2026
Remove first-upload and missing-base onboarding messages to match the
size analysis pattern, which doesn't have these Emerge-specific flows.
Remove format_snapshot_pr_comment_solo in favor of handling the no-base
case within format_snapshot_pr_comment. When there's no base artifact,
the same comparison table is shown with "N uploaded" in the status
column, matching how size analysis handles missing bases.
@runningcode runningcode marked this pull request as ready for review April 7, 2026 14:08
@runningcode runningcode requested a review from a team as a code owner April 7, 2026 14:08
from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
from sentry.preprod.url_utils import get_preprod_artifact_comparison_url, get_preprod_artifact_url

_HEADER = "## Sentry Snapshot Testing"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to "Sentry Snapshots" or something else. WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Substantial logic duplication with status checks snapshot template
    • Extracted shared table-building logic into snapshot_template_utils.py, eliminating ~60 lines of duplication while preserving all existing behavior.

Create PR

Or push these changes by commenting:

@cursor push ab32820f56
Preview (ab32820f56)
diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py b/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py
--- a/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py
+++ b/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py
@@ -1,10 +1,14 @@
 from __future__ import annotations
 
-from django.utils.translation import gettext_lazy as _
-
 from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval
 from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
-from sentry.preprod.url_utils import get_preprod_artifact_comparison_url, get_preprod_artifact_url
+from sentry.preprod.vcs.snapshot_template_utils import (
+    format_name_cell,
+    format_section_cell,
+    get_app_display_info,
+    get_artifact_url,
+    get_comparison_status,
+)
 
 _HEADER = "## Sentry Snapshot Testing"
 _PROCESSING_STATUS = "\u23f3 Processing"
@@ -55,30 +59,17 @@
             table_rows.append(f"| {name_cell} | - | - | - | - | - | \u274c Comparison failed |")
         else:
             base_artifact = base_artifact_map.get(artifact.id)
-            artifact_url = (
-                get_preprod_artifact_comparison_url(
-                    artifact, base_artifact, comparison_type="snapshots"
-                )
-                if base_artifact
-                else get_preprod_artifact_url(artifact, view_type="snapshots")
-            )
+            metrics = snapshot_metrics_map.get(artifact.id)
+            artifact_url = get_artifact_url(artifact, base_artifact, metrics)
+            status = get_comparison_status(artifact.id, changes_map, approvals_map)
 
-            has_changes = changes_map.get(artifact.id, False)
-            is_approved = approvals_map is not None and artifact.id in approvals_map
-            if has_changes and is_approved:
-                status = "\u2705 Approved"
-            elif has_changes:
-                status = "\u23f3 Needs approval"
-            else:
-                status = "\u2705 Unchanged"
-
             table_rows.append(
                 f"| {name_cell}"
-                f" | {_section_cell(comparison.images_added, 'added', artifact_url)}"
-                f" | {_section_cell(comparison.images_removed, 'removed', artifact_url)}"
-                f" | {_section_cell(comparison.images_changed, 'changed', artifact_url)}"
-                f" | {_section_cell(comparison.images_renamed, 'renamed', artifact_url)}"
-                f" | {_section_cell(comparison.images_unchanged, 'unchanged', artifact_url)}"
+                f" | {format_section_cell(comparison.images_added, 'added', artifact_url)}"
+                f" | {format_section_cell(comparison.images_removed, 'removed', artifact_url)}"
+                f" | {format_section_cell(comparison.images_changed, 'changed', artifact_url)}"
+                f" | {format_section_cell(comparison.images_renamed, 'renamed', artifact_url)}"
+                f" | {format_section_cell(comparison.images_unchanged, 'unchanged', artifact_url)}"
                 f" | {status} |"
             )
 
@@ -95,35 +86,8 @@
     snapshot_metrics_map: dict[int, PreprodSnapshotMetrics],
     base_artifact_map: dict[int, PreprodArtifact],
 ) -> str:
-    app_display, app_id = _app_display_info(artifact)
+    app_display, app_id = get_app_display_info(artifact)
     metrics = snapshot_metrics_map.get(artifact.id)
     base_artifact = base_artifact_map.get(artifact.id)
-
-    if base_artifact and metrics:
-        artifact_url = get_preprod_artifact_comparison_url(
-            artifact, base_artifact, comparison_type="snapshots"
-        )
-    else:
-        artifact_url = get_preprod_artifact_url(artifact, view_type="snapshots")
-
-    return _format_name_cell(app_display, app_id, artifact_url)
-
-
-def _app_display_info(artifact: PreprodArtifact) -> tuple[str, str]:
-    mobile_app_info = getattr(artifact, "mobile_app_info", None)
-    app_name = mobile_app_info.app_name if mobile_app_info else None
-    app_display = app_name or artifact.app_id or str(_("Unknown App"))
-    app_id = artifact.app_id or ""
-    return app_display, app_id
-
-
-def _format_name_cell(app_display: str, app_id: str, url: str) -> str:
-    if app_id:
-        return f"[{app_display}]({url})<br>`{app_id}`"
-    return f"[{app_display}]({url})"
-
-
-def _section_cell(count: int, section: str, artifact_url: str) -> str:
-    if count > 0:
-        return f"[{count}]({artifact_url}?section={section})"
-    return str(count)
+    artifact_url = get_artifact_url(artifact, base_artifact, metrics)
+    return format_name_cell(app_display, app_id, artifact_url)

diff --git a/src/sentry/preprod/vcs/snapshot_template_utils.py b/src/sentry/preprod/vcs/snapshot_template_utils.py
new file mode 100644
--- /dev/null
+++ b/src/sentry/preprod/vcs/snapshot_template_utils.py
@@ -1,0 +1,59 @@
+from __future__ import annotations
+
+from django.utils.translation import gettext_lazy as _
+
+from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval
+from sentry.preprod.snapshots.models import PreprodSnapshotMetrics
+from sentry.preprod.url_utils import get_preprod_artifact_comparison_url, get_preprod_artifact_url
+
+
+def get_app_display_info(artifact: PreprodArtifact) -> tuple[str, str]:
+    """Extract app display name and app ID from artifact."""
+    mobile_app_info = getattr(artifact, "mobile_app_info", None)
+    app_name = mobile_app_info.app_name if mobile_app_info else None
+    app_display = app_name or artifact.app_id or str(_("Unknown App"))
+    app_id = artifact.app_id or ""
+    return app_display, app_id
+
+
+def format_name_cell(app_display: str, app_id: str, url: str) -> str:
+    """Format the name cell with app display name, app ID, and URL."""
+    if app_id:
+        return f"[{app_display}]({url})<br>`{app_id}`"
+    return f"[{app_display}]({url})"
+
+
+def get_artifact_url(
+    artifact: PreprodArtifact,
+    base_artifact: PreprodArtifact | None,
+    metrics: PreprodSnapshotMetrics | None,
+) -> str:
+    """Get the appropriate URL for an artifact (comparison or standalone)."""
+    if base_artifact and metrics:
+        return get_preprod_artifact_comparison_url(
+            artifact, base_artifact, comparison_type="snapshots"
+        )
+    return get_preprod_artifact_url(artifact, view_type="snapshots")
+
+
+def format_section_cell(count: int, section: str, artifact_url: str) -> str:
+    """Format a section cell with count and optional link."""
+    if count > 0:
+        return f"[{count}]({artifact_url}?section={section})"
+    return str(count)
+
+
+def get_comparison_status(
+    artifact_id: int,
+    changes_map: dict[int, bool],
+    approvals_map: dict[int, PreprodComparisonApproval] | None = None,
+) -> str:
+    """Determine the status string for a comparison."""
+    has_changes = changes_map.get(artifact_id, False)
+    is_approved = approvals_map is not None and artifact_id in approvals_map
+    if has_changes and is_approved:
+        return "\u2705 Approved"
+    elif has_changes:
+        return "\u23f3 Needs approval"
+    else:
+        return "\u2705 Unchanged"

diff --git a/src/sentry/preprod/vcs/status_checks/snapshots/templates.py b/src/sentry/preprod/vcs/status_checks/snapshots/templates.py
--- a/src/sentry/preprod/vcs/status_checks/snapshots/templates.py
+++ b/src/sentry/preprod/vcs/status_checks/snapshots/templates.py
@@ -6,7 +6,14 @@
 from sentry.integrations.source_code_management.status_check import StatusCheckStatus
 from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval
 from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
-from sentry.preprod.url_utils import get_preprod_artifact_comparison_url, get_preprod_artifact_url
+from sentry.preprod.url_utils import get_preprod_artifact_url
+from sentry.preprod.vcs.snapshot_template_utils import (
+    format_name_cell,
+    format_section_cell,
+    get_app_display_info,
+    get_artifact_url,
+    get_comparison_status,
+)
 
 _SNAPSHOT_TITLE_BASE = _("Snapshot Testing")
 _PROCESSING_STATUS = "⏳ Processing"
@@ -193,19 +200,10 @@
     table_rows = []
 
     for artifact in artifacts:
-        mobile_app_info = getattr(artifact, "mobile_app_info", None)
-        app_name = mobile_app_info.app_name if mobile_app_info else None
-        app_display = app_name or artifact.app_id or str(_("Unknown App"))
-        app_id = artifact.app_id or ""
-
+        app_display, app_id = get_app_display_info(artifact)
         artifact_url = get_preprod_artifact_url(artifact, view_type="snapshots")
+        name_cell = format_name_cell(app_display, app_id, artifact_url)
 
-        name_cell = (
-            f"[{app_display}]({artifact_url})<br>`{app_id}`"
-            if app_id
-            else f"[{app_display}]({artifact_url})"
-        )
-
         metrics = snapshot_metrics_map.get(artifact.id)
         if not metrics:
             table_rows.append(f"| {name_cell} | - | {_PROCESSING_STATUS} |")
@@ -229,27 +227,12 @@
     table_rows = []
 
     for artifact in artifacts:
-        mobile_app_info = getattr(artifact, "mobile_app_info", None)
-        app_name = mobile_app_info.app_name if mobile_app_info else None
-        app_display = app_name or artifact.app_id or str(_("Unknown App"))
-        app_id = artifact.app_id or ""
-
+        app_display, app_id = get_app_display_info(artifact)
         metrics = snapshot_metrics_map.get(artifact.id)
         base_artifact = base_artifact_map.get(artifact.id)
+        artifact_url = get_artifact_url(artifact, base_artifact, metrics)
+        name_cell = format_name_cell(app_display, app_id, artifact_url)
 
-        if base_artifact and metrics:
-            artifact_url = get_preprod_artifact_comparison_url(
-                artifact, base_artifact, comparison_type="snapshots"
-            )
-        else:
-            artifact_url = get_preprod_artifact_url(artifact, view_type="snapshots")
-
-        name_cell = (
-            f"[{app_display}]({artifact_url})<br>`{app_id}`"
-            if app_id
-            else f"[{app_display}]({artifact_url})"
-        )
-
         if not metrics:
             table_rows.append(f"| {name_cell} | - | - | - | - | - | {_PROCESSING_STATUS} |")
             continue
@@ -265,33 +248,15 @@
         ):
             table_rows.append(f"| {name_cell} | - | - | - | - | - | {_PROCESSING_STATUS} |")
         else:
-            added = comparison.images_added
-            removed = comparison.images_removed
-            modified = comparison.images_changed
-            renamed = comparison.images_renamed
-            unchanged = comparison.images_unchanged
-            has_changes = changes_map.get(artifact.id, False)
-            is_approved = approvals_map is not None and artifact.id in approvals_map
-            if has_changes and is_approved:
-                status = "✅ Approved"
-            elif has_changes:
-                status = "⏳ Needs approval"
-            else:
-                status = "✅ Unchanged"
+            status = get_comparison_status(artifact.id, changes_map, approvals_map)
 
-            def _section_cell(count: int, section: str) -> str:
-                if count > 0:
-                    section_url = f"{artifact_url}?section={section}"
-                    return f"[{count}]({section_url})"
-                return str(count)
-
             table_rows.append(
                 f"| {name_cell}"
-                f" | {_section_cell(added, 'added')}"
-                f" | {_section_cell(removed, 'removed')}"
-                f" | {_section_cell(modified, 'changed')}"
-                f" | {_section_cell(renamed, 'renamed')}"
-                f" | {_section_cell(unchanged, 'unchanged')}"
+                f" | {format_section_cell(comparison.images_added, 'added', artifact_url)}"
+                f" | {format_section_cell(comparison.images_removed, 'removed', artifact_url)}"
+                f" | {format_section_cell(comparison.images_changed, 'changed', artifact_url)}"
+                f" | {format_section_cell(comparison.images_renamed, 'renamed', artifact_url)}"
+                f" | {format_section_cell(comparison.images_unchanged, 'unchanged', artifact_url)}"
                 f" | {status} |"
             )

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 15b3213. Configure here.

Extract shared helpers (_name_cell, _section_cell, _app_display_info,
_format_name_cell) and constants (PROCESSING_STATUS,
COMPARISON_TABLE_HEADER) from snapshot_templates.py so that
_format_snapshot_summary in status_checks can reuse them instead of
duplicating the logic.
@runningcode runningcode merged commit e6d0860 into master Apr 9, 2026
56 checks passed
@runningcode runningcode deleted the no/snapshot-pr-comment-templates branch April 9, 2026 06:59
george-sentry pushed a commit that referenced this pull request Apr 9, 2026
Add formatting for snapshot PR comments that will be posted to GitHub
PRs when snapshot comparisons complete. Single function handles all
cases — comparison results, no-base uploads, processing, and failures —
using the same table format throughout, consistent with how size
analysis works.

This is the first of three PRs to implement snapshot PR comments
(EME-999). Templates only — no runtime effect. The Celery task and
trigger wiring will follow in subsequent PRs.

Refs EME-999
runningcode added a commit that referenced this pull request Apr 14, 2026
Add Celery task that posts/updates GitHub PR comments for snapshot
comparisons, and wire it into all existing trigger points.

**Task** (`snapshot_tasks.py`): Follows the build distribution PR
comment pattern — locks CommitComparison rows to prevent duplicates,
finds existing comments across PR commits, creates or updates via GitHub
API, stores comment_id in `extras["pr_comments"]["snapshots"]`. Gated by
feature flag `organizations:preprod-snapshot-pr-comments` and project
option `sentry:preprod_snapshot_pr_comments_enabled` (both already
registered).

**Wiring**: Triggered alongside the snapshot status check task at five
entry points:
- Comparison completion and failure (`snapshots/tasks.py`)
- Upload completion (`preprod_artifact_snapshot.py`)
- Approval via API (`preprod_artifact_approve.py`)
- Approval via GitHub check run button (`github_check_run.py`)
- Rerun status checks (`preprod_artifact_rerun_status_checks.py`)

I might have missed something so let me know if this should be added
somewhere else!

Here's what this looks like using the test script:
[link](runningcode/hackernews#1)
<img width="910" height="373" alt="image"
src="https://github.com/user-attachments/assets/04569cbf-268d-437b-b0c4-4f2b86541226"
/>


Depends on #112353.

Refs EME-999
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants