feat(preprod): Use one relevant-filtered link per snapshot row

src/sentry/preprod/vcs/pr_comments/snapshot_templates.py

@@ -46,14 +46,24 @@ def format_snapshot_pr_comment(
     total_errored = 0
 
     for artifact in artifacts:
-        name_cell = _name_cell(artifact, snapshot_metrics_map, base_artifact_map)
         metrics = snapshot_metrics_map.get(artifact.id)
+        comparison = comparisons_map.get(metrics.id) if metrics else None
+        include_selected_types = comparison is not None and comparison.state not in (
+            PreprodSnapshotComparison.State.PENDING,
+            PreprodSnapshotComparison.State.PROCESSING,
+            PreprodSnapshotComparison.State.FAILED,
+        )
+        name_cell = _name_cell(
+            artifact,
+            snapshot_metrics_map,
+            base_artifact_map,
+            comparison=comparison if include_selected_types else None,
+        )
 
         if not metrics:
             table_rows.append(f"| {name_cell} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
             continue
 
-        comparison = comparisons_map.get(metrics.id)
         has_base = artifact.id in base_artifact_map
 
         if not comparison and not has_base:
@@ -75,15 +85,6 @@ def format_snapshot_pr_comment(
         elif comparison.state == PreprodSnapshotComparison.State.FAILED:
             table_rows.append(f"| {name_cell} | - | - | - | - | - | - | ❌ 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")
-            )
-
             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:
@@ -95,12 +96,12 @@ def format_snapshot_pr_comment(
 
             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" | {_section_cell(comparison.images_skipped, 'skipped', artifact_url)}"
+                f" | {comparison.images_added}"
+                f" | {comparison.images_removed}"
+                f" | {comparison.images_changed}"
+                f" | {comparison.images_renamed}"
+                f" | {comparison.images_unchanged}"
+                f" | {comparison.images_skipped}"
                 f" | {status} |"
             )
             total_errored += comparison.images_errored
@@ -118,6 +119,7 @@ def _name_cell(
     artifact: PreprodArtifact,
     snapshot_metrics_map: dict[int, PreprodSnapshotMetrics],
     base_artifact_map: dict[int, PreprodArtifact],
+    comparison: PreprodSnapshotComparison | None = None,
 ) -> str:
     app_display, app_id = _app_display_info(artifact)
     metrics = snapshot_metrics_map.get(artifact.id)
@@ -130,6 +132,9 @@ def _name_cell(
     else:
         artifact_url = get_preprod_artifact_url(artifact, view_type="snapshots")
 
+    if comparison is not None:
+        artifact_url += _selected_types_query(comparison)
+
     return _format_name_cell(app_display, app_id, artifact_url)
 
 
@@ -147,10 +152,17 @@ def _format_name_cell(app_display: str, app_id: str, url: str) -> str:
     return f"[{app_display}]({url})"
 
 
-def _section_cell(count: int, section: str, artifact_url: str) -> str:
-    if count > 0:
-        return f"[{count}]({artifact_url}?selectedTypes={section})"
-    return str(count)
+def _selected_types_query(comparison: PreprodSnapshotComparison) -> str:
+    counts = (
+        ("added", comparison.images_added),
+        ("removed", comparison.images_removed),
+        ("changed", comparison.images_changed),
+        ("renamed", comparison.images_renamed),
+    )
+    selected = [name for name, count in counts if count > 0]
+    if not selected:
+        return ""
+    return f"?selectedTypes={','.join(selected)}"
 
 
 def _format_settings_link(project: Project) -> str:

src/sentry/preprod/vcs/status_checks/snapshots/templates.py

@@ -7,14 +7,13 @@
 from sentry.models.project import Project
 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.pr_comments.snapshot_templates import (
     COMPARISON_TABLE_HEADER,
     PROCESSING_STATUS,
     _app_display_info,
     _format_name_cell,
     _name_cell,
-    _section_cell,
     format_errored_note,
 )
 
@@ -284,14 +283,23 @@ def _format_snapshot_summary(
     table_rows = []
 
     for artifact in artifacts:
-        name = _name_cell(artifact, snapshot_metrics_map, base_artifact_map)
-
         metrics = snapshot_metrics_map.get(artifact.id)
+        comparison = comparisons_map.get(metrics.id) if metrics else None
+        include_selected_types = comparison is not None and comparison.state not in (
+            PreprodSnapshotComparison.State.PENDING,
+            PreprodSnapshotComparison.State.PROCESSING,
+        )
+        name = _name_cell(
+            artifact,
+            snapshot_metrics_map,
+            base_artifact_map,
+            comparison=comparison if include_selected_types else None,
+        )
+
         if not metrics:
             table_rows.append(f"| {name} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
             continue
 
-        comparison = comparisons_map.get(metrics.id)
         if not comparison:
             table_rows.append(f"| {name} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
             continue
@@ -302,15 +310,6 @@ def _format_snapshot_summary(
         ):
             table_rows.append(f"| {name} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
         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")
-            )
-
             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:
@@ -322,12 +321,12 @@ def _format_snapshot_summary(
 
             table_rows.append(
                 f"| {name}"
-                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" | {_section_cell(comparison.images_skipped, 'skipped', artifact_url)}"
+                f" | {comparison.images_added}"
+                f" | {comparison.images_removed}"
+                f" | {comparison.images_changed}"
+                f" | {comparison.images_renamed}"
+                f" | {comparison.images_unchanged}"
+                f" | {comparison.images_skipped}"
                 f" | {status} |"
             )
 

tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py

@@ -1,10 +1,13 @@
 from __future__ import annotations
 
+from types import SimpleNamespace
+
 import pytest
 
 from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval
 from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
 from sentry.preprod.vcs.pr_comments.snapshot_templates import (
+    _selected_types_query,
     format_missing_base_snapshot_pr_comment,
     format_snapshot_pr_comment,
     format_solo_snapshot_pr_comment,
@@ -14,6 +17,32 @@
 from sentry.testutils.silo import cell_silo_test
 
 
+def _comparison(
+    *, changed: int = 0, added: int = 0, removed: int = 0, renamed: int = 0
+) -> PreprodSnapshotComparison:
+    return SimpleNamespace(
+        images_changed=changed,
+        images_added=added,
+        images_removed=removed,
+        images_renamed=renamed,
+    )  # type: ignore[return-value]
+
+
+def test_selected_types_query_all_categories() -> None:
+    assert (
+        _selected_types_query(_comparison(changed=1, added=2, removed=3, renamed=4))
+        == "?selectedTypes=added,removed,changed,renamed"
+    )
+
+
+def test_selected_types_query_subset_preserves_order() -> None:
+    assert _selected_types_query(_comparison(changed=1, added=1)) == "?selectedTypes=added,changed"
+
+
+def test_selected_types_query_none_changed_is_empty() -> None:
+    assert _selected_types_query(_comparison()) == ""
+
+
 class SnapshotPrCommentTestBase(TestCase):
     def setUp(self) -> None:
         super().setUp()
@@ -251,10 +280,9 @@ def test_no_changes_shows_unchanged(self) -> None:
         assert "Unchanged" in result
         assert "My App" in result
         assert "`com.example.app`" in result
-        # Zero counts are plain text, non-zero unchanged is linked
         assert "| 0 | 0 | 0 | 0 |" in result
         assert "| 0 | ✅ Unchanged |" in result
-        assert "?selectedTypes=unchanged" in result
+        assert "?selectedTypes=" not in result
 
     def test_changes_show_needs_approval(self) -> None:
         head_artifact, head_metrics = self._create_artifact_with_metrics()
@@ -280,10 +308,7 @@ def test_changes_show_needs_approval(self) -> None:
         )
 
         assert "Needs approval" in result
-        assert "?selectedTypes=added" in result
-        assert "?selectedTypes=removed" in result
-        assert "?selectedTypes=changed" in result
-        assert "?selectedTypes=renamed" in result
+        assert "?selectedTypes=added,removed,changed,renamed" in result
 
     def test_approved_shows_approved_status(self) -> None:
         head_artifact, head_metrics = self._create_artifact_with_metrics()

tests/sentry/preprod/vcs/status_checks/snapshots/test_templates.py

@@ -725,9 +725,7 @@ def test_full_success_summary_format(self) -> None:
             "| Name | Added | Removed | Changed | Renamed | Unchanged | Skipped | Status |\n"
             "| :--- | :---: | :---: | :---: | :---: | :---: | :---: | :---: |\n"
             f"| [My App]({artifact_url})<br>`com.example.app`"
-            f" | 0 | 0 | 0 | 0"
-            f" | [{15}]({artifact_url}?selectedTypes=unchanged)"
-            f" | 0"
+            f" | 0 | 0 | 0 | 0 | 15 | 0"
             f" | ✅ Unchanged |"
             f"\n\n{configure_link}"
         )
@@ -771,13 +769,8 @@ def test_full_failure_summary_format(self) -> None:
         expected = (
             "| Name | Added | Removed | Changed | Renamed | Unchanged | Skipped | Status |\n"
             "| :--- | :---: | :---: | :---: | :---: | :---: | :---: | :---: |\n"
-            f"| [My App]({artifact_url})<br>`com.example.app`"
-            f" | [{1}]({artifact_url}?selectedTypes=added)"
-            f" | [{2}]({artifact_url}?selectedTypes=removed)"
-            f" | [{3}]({artifact_url}?selectedTypes=changed)"
-            f" | [{1}]({artifact_url}?selectedTypes=renamed)"
-            f" | [{4}]({artifact_url}?selectedTypes=unchanged)"
-            f" | 0"
+            f"| [My App]({artifact_url}?selectedTypes=added,removed,changed,renamed)<br>`com.example.app`"
+            f" | 1 | 2 | 3 | 1 | 4 | 0"
             f" | ⏳ Needs approval |"
             f"\n\n{configure_link}"
         )
@@ -1143,13 +1136,8 @@ def test_approved_summary_table_format(self) -> None:
         expected = (
             "| Name | Added | Removed | Changed | Renamed | Unchanged | Skipped | Status |\n"
             "| :--- | :---: | :---: | :---: | :---: | :---: | :---: | :---: |\n"
-            f"| [My App]({artifact_url})<br>`com.example.app`"
-            f" | [{1}]({artifact_url}?selectedTypes=added)"
-            f" | [{2}]({artifact_url}?selectedTypes=removed)"
-            f" | [{3}]({artifact_url}?selectedTypes=changed)"
-            f" | [{1}]({artifact_url}?selectedTypes=renamed)"
-            f" | [{4}]({artifact_url}?selectedTypes=unchanged)"
-            f" | 0"
+            f"| [My App]({artifact_url}?selectedTypes=added,removed,changed,renamed)<br>`com.example.app`"
+            f" | 1 | 2 | 3 | 1 | 4 | 0"
             f" | ✅ Approved |"
             f"\n\n{configure_link}"
         )