feat(signals): rasterize session problem moments as GIF previews#55463
feat(signals): rasterize session problem moments as GIF previews#55463
Conversation
|
Hey @Twixes! 👋\nThis pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future. |
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
Prompt To Fix All With AIThis is a comment left during a code review.
Path: products/signals/backend/views.py
Line: 114-116
Comment:
**`content` field omitted from `.only()` but accessed by `has_content`**
`has_content` is defined as `return self.content is not None or self.content_location is not None`. Since `"content"` is not included in the `.only()` projection, accessing `self.content` when `content_location` is `None` will trigger a deferred per-row query (N+1). For S3-backed GIF assets this is short-circuited, but including `"content"` would make the intent explicit and safe for all asset types.
```suggestion
assets = ExportedAsset.objects.filter(id__in=asset_ids).only(
"id", "content", "content_location", "export_format", "created_at", "export_context"
)
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/temporal/session_replay/session_summary/activities/a6a_emit_session_problem_signals.py
Line: 58-75
Comment:
**Docstring overpromises minimum-duration guarantee**
The docstring says "Ensures the rendered clip is at least `MIN_MOMENT_PREVIEW_DURATION_S` seconds", but when `start_time_s` is clamped to `0` (e.g. a segment at 5–10 s), `end_time_s` is kept at `midpoint + half = 22.5 s`, yielding a 22.5 s clip — less than the 30 s minimum. The test `test_clamps_start_to_zero_for_early_segments` explicitly asserts this behaviour (`end_offset_s == 22.5`), so either the docstring should drop the guarantee, or `end_time_s` should be extended to compensate when the start is clamped.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/temporal/tests/session_replay/session_summary/test_emit_session_problem_signals.py
Line: 85-230
Comment:
**Non-parameterised async tests repeat setup boilerplate**
The four `TestRasterizeMomentPreview` methods share identical mock setup (mock asset, mock client, two patches) and only differ in input values and expected `export_context` assertions. Prefer `@pytest.mark.parametrize` to follow the project convention of parameterised tests and to reduce the copy-paste noise.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(signals): rasterize session problem..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assets = ExportedAsset.objects.filter(id__in=asset_ids).only( | ||
| "id", "content_location", "export_format", "created_at", "export_context" | ||
| ) |
There was a problem hiding this comment.
content field omitted from .only() but accessed by has_content
has_content is defined as return self.content is not None or self.content_location is not None. Since "content" is not included in the .only() projection, accessing self.content when content_location is None will trigger a deferred per-row query (N+1). For S3-backed GIF assets this is short-circuited, but including "content" would make the intent explicit and safe for all asset types.
| assets = ExportedAsset.objects.filter(id__in=asset_ids).only( | |
| "id", "content_location", "export_format", "created_at", "export_context" | |
| ) | |
| assets = ExportedAsset.objects.filter(id__in=asset_ids).only( | |
| "id", "content", "content_location", "export_format", "created_at", "export_context" | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/signals/backend/views.py
Line: 114-116
Comment:
**`content` field omitted from `.only()` but accessed by `has_content`**
`has_content` is defined as `return self.content is not None or self.content_location is not None`. Since `"content"` is not included in the `.only()` projection, accessing `self.content` when `content_location` is `None` will trigger a deferred per-row query (N+1). For S3-backed GIF assets this is short-circuited, but including `"content"` would make the intent explicit and safe for all asset types.
```suggestion
assets = ExportedAsset.objects.filter(id__in=asset_ids).only(
"id", "content", "content_location", "export_format", "created_at", "export_context"
)
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Claude speaking: Addressed — "content" is now included in the .only() projection at line 117 of products/signals/backend/views.py, alongside "id", "content_location", "export_format", "created_at", and "export_context". This prevents the deferred N+1 query when has_content checks self.content.
There was a problem hiding this comment.
Claude speaking: Addressed — "content" is now included in the .only() projection at line 117 of products/signals/backend/views.py, alongside "id", "content_location", "export_format", "created_at", and "export_context". This prevents the deferred N+1 query when has_content checks self.content.
| async def _rasterize_moment_preview( | ||
| team_id: int, | ||
| session_id: str, | ||
| start_time_s: float, | ||
| end_time_s: float, | ||
| ) -> int | None: | ||
| """Create a GIF preview of a session moment and return the ExportedAsset ID. | ||
|
|
||
| Ensures the rendered clip is at least MIN_MOMENT_PREVIEW_DURATION_S seconds. | ||
| Returns None if rasterization fails. | ||
| """ | ||
| duration = end_time_s - start_time_s | ||
| if duration < MIN_MOMENT_PREVIEW_DURATION_S: | ||
| # Expand symmetrically around the midpoint to reach the minimum | ||
| midpoint = (start_time_s + end_time_s) / 2 | ||
| half = MIN_MOMENT_PREVIEW_DURATION_S / 2 | ||
| start_time_s = max(0, midpoint - half) | ||
| end_time_s = midpoint + half |
There was a problem hiding this comment.
The docstring says "Ensures the rendered clip is at least MIN_MOMENT_PREVIEW_DURATION_S seconds", but when start_time_s is clamped to 0 (e.g. a segment at 5–10 s), end_time_s is kept at midpoint + half = 22.5 s, yielding a 22.5 s clip — less than the 30 s minimum. The test test_clamps_start_to_zero_for_early_segments explicitly asserts this behaviour (end_offset_s == 22.5), so either the docstring should drop the guarantee, or end_time_s should be extended to compensate when the start is clamped.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/temporal/session_replay/session_summary/activities/a6a_emit_session_problem_signals.py
Line: 58-75
Comment:
**Docstring overpromises minimum-duration guarantee**
The docstring says "Ensures the rendered clip is at least `MIN_MOMENT_PREVIEW_DURATION_S` seconds", but when `start_time_s` is clamped to `0` (e.g. a segment at 5–10 s), `end_time_s` is kept at `midpoint + half = 22.5 s`, yielding a 22.5 s clip — less than the 30 s minimum. The test `test_clamps_start_to_zero_for_early_segments` explicitly asserts this behaviour (`end_offset_s == 22.5`), so either the docstring should drop the guarantee, or `end_time_s` should be extended to compensate when the start is clamped.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Claude speaking: Addressed — the docstring now reads: 'Expands the clip to at least MIN_MOMENT_PREVIEW_DURATION_S seconds where possible, clamping start to 0 and compensating the end when near the recording start.' This accurately reflects both the clamping behavior (start floored at 0) and the compensation (end extended to MIN_MOMENT_PREVIEW_DURATION_S when start is clamped), rather than overpromising an unconditional guarantee.
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| class TestRasterizeMomentPreview: | ||
| @pytest.mark.asyncio | ||
| async def test_creates_asset_and_triggers_workflow(self): | ||
| mock_asset = MagicMock() | ||
| mock_asset.id = 42 | ||
|
|
||
| mock_client = AsyncMock() | ||
| mock_client.execute_workflow = AsyncMock(return_value=None) | ||
|
|
||
| with ( | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.ExportedAsset.objects" | ||
| ) as mock_objects, | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.async_connect", | ||
| return_value=mock_client, | ||
| ), | ||
| ): | ||
| mock_objects.acreate = AsyncMock(return_value=mock_asset) | ||
|
|
||
| result = await _rasterize_moment_preview( | ||
| team_id=1, | ||
| session_id="sess-123", | ||
| start_time_s=60.0, | ||
| end_time_s=120.0, | ||
| ) | ||
|
|
||
| assert result == 42 | ||
| mock_objects.acreate.assert_called_once() | ||
| create_kwargs = mock_objects.acreate.call_args[1] | ||
| assert create_kwargs["team_id"] == 1 | ||
| assert create_kwargs["export_format"] == MOMENT_PREVIEW_EXPORT_FORMAT | ||
| assert create_kwargs["export_context"]["session_recording_id"] == "sess-123" | ||
| assert create_kwargs["export_context"]["start_offset_s"] == 60.0 | ||
| assert create_kwargs["export_context"]["end_offset_s"] == 120.0 | ||
| assert create_kwargs["export_context"]["playback_speed"] == 1 | ||
|
|
||
| mock_client.execute_workflow.assert_called_once() | ||
| call_args = mock_client.execute_workflow.call_args | ||
| assert call_args[0][0] == "rasterize-recording" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_expands_short_segment_to_minimum_duration(self): | ||
| mock_asset = MagicMock() | ||
| mock_asset.id = 99 | ||
|
|
||
| mock_client = AsyncMock() | ||
| mock_client.execute_workflow = AsyncMock(return_value=None) | ||
|
|
||
| with ( | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.ExportedAsset.objects" | ||
| ) as mock_objects, | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.async_connect", | ||
| return_value=mock_client, | ||
| ), | ||
| ): | ||
| mock_objects.acreate = AsyncMock(return_value=mock_asset) | ||
|
|
||
| # 10s segment should be expanded to MIN_MOMENT_PREVIEW_DURATION_S | ||
| result = await _rasterize_moment_preview( | ||
| team_id=1, | ||
| session_id="sess-short", | ||
| start_time_s=50.0, | ||
| end_time_s=60.0, | ||
| ) | ||
|
|
||
| assert result == 99 | ||
| create_kwargs = mock_objects.acreate.call_args[1] | ||
| ctx = create_kwargs["export_context"] | ||
| # Midpoint is 55, half of 30 is 15, so start=40, end=70 | ||
| assert ctx["start_offset_s"] == 40.0 | ||
| assert ctx["end_offset_s"] == 70.0 | ||
| actual_duration = ctx["end_offset_s"] - ctx["start_offset_s"] | ||
| assert actual_duration >= MIN_MOMENT_PREVIEW_DURATION_S | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_clamps_start_to_zero_for_early_segments(self): | ||
| mock_asset = MagicMock() | ||
| mock_asset.id = 77 | ||
|
|
||
| mock_client = AsyncMock() | ||
| mock_client.execute_workflow = AsyncMock(return_value=None) | ||
|
|
||
| with ( | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.ExportedAsset.objects" | ||
| ) as mock_objects, | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.async_connect", | ||
| return_value=mock_client, | ||
| ), | ||
| ): | ||
| mock_objects.acreate = AsyncMock(return_value=mock_asset) | ||
|
|
||
| # Segment at the very start (5s–10s) — midpoint 7.5, half of 30 is 15 | ||
| # start would be -7.5, clamped to 0 | ||
| result = await _rasterize_moment_preview( | ||
| team_id=1, | ||
| session_id="sess-early", | ||
| start_time_s=5.0, | ||
| end_time_s=10.0, | ||
| ) | ||
|
|
||
| assert result == 77 | ||
| ctx = mock_objects.acreate.call_args[1]["export_context"] | ||
| assert ctx["start_offset_s"] == 0 | ||
| assert ctx["end_offset_s"] == 22.5 # midpoint 7.5 + 15 | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_returns_none_and_cleans_up_on_workflow_failure(self): | ||
| mock_asset = MagicMock() | ||
| mock_asset.id = 50 | ||
|
|
||
| mock_client = AsyncMock() | ||
| mock_client.execute_workflow = AsyncMock(side_effect=RuntimeError("workflow failed")) | ||
|
|
||
| mock_filter = AsyncMock() | ||
|
|
||
| with ( | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.ExportedAsset.objects" | ||
| ) as mock_objects, | ||
| patch( | ||
| "posthog.temporal.session_replay.session_summary.activities.a6a_emit_session_problem_signals.async_connect", | ||
| return_value=mock_client, | ||
| ), | ||
| ): | ||
| mock_objects.acreate = AsyncMock(return_value=mock_asset) | ||
| mock_objects.filter = MagicMock(return_value=mock_filter) | ||
| mock_filter.adelete = AsyncMock(return_value=None) | ||
|
|
||
| result = await _rasterize_moment_preview( | ||
| team_id=1, | ||
| session_id="sess-fail", | ||
| start_time_s=0.0, | ||
| end_time_s=60.0, | ||
| ) | ||
|
|
||
| assert result is None | ||
| mock_objects.filter.assert_called_once_with(id=50) | ||
| mock_filter.adelete.assert_called_once() |
There was a problem hiding this comment.
The four TestRasterizeMomentPreview methods share identical mock setup (mock asset, mock client, two patches) and only differ in input values and expected export_context assertions. Prefer @pytest.mark.parametrize to follow the project convention of parameterised tests and to reduce the copy-paste noise.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/temporal/tests/session_replay/session_summary/test_emit_session_problem_signals.py
Line: 85-230
Comment:
**Non-parameterised async tests repeat setup boilerplate**
The four `TestRasterizeMomentPreview` methods share identical mock setup (mock asset, mock client, two patches) and only differ in input values and expected `export_context` assertions. Prefer `@pytest.mark.parametrize` to follow the project convention of parameterised tests and to reduce the copy-paste noise.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Claude speaking: Addressed — the four duplicate test methods have been replaced with a single @pytest.mark.parametrize-decorated async test test_export_context_offsets with three parameter sets (no_expansion, symmetric_expansion, clamp_start_extend_end). The shared mock setup appears once in the test body, and the test also verifies the minimum-duration invariant (end_offset_s - start_offset_s >= MIN_MOMENT_PREVIEW_DURATION_S) for all cases.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if duration < MIN_MOMENT_PREVIEW_DURATION_S: | ||
| # Expand symmetrically around the midpoint to reach the minimum | ||
| midpoint = (start_time_s + end_time_s) / 2 | ||
| half = MIN_MOMENT_PREVIEW_DURATION_S / 2 | ||
| start_time_s = max(0, midpoint - half) | ||
| end_time_s = midpoint + half |
There was a problem hiding this comment.
Critical bug: The minimum duration guarantee is broken when segments are near the start of the recording. When start_time_s is clamped to 0 (line 74), the resulting duration end_time_s - start_time_s will be less than MIN_MOMENT_PREVIEW_DURATION_S (30 seconds).
Example: For a segment at 5s-10s:
- Midpoint = 7.5s
- Calculated start = 7.5 - 15 = -7.5 → clamped to 0
- Calculated end = 7.5 + 15 = 22.5s
- Actual duration = 22.5s < 30s ❌
Fix: After clamping start to 0, adjust end to maintain the minimum duration:
duration = end_time_s - start_time_s
if duration < MIN_MOMENT_PREVIEW_DURATION_S:
midpoint = (start_time_s + end_time_s) / 2
half = MIN_MOMENT_PREVIEW_DURATION_S / 2
start_time_s = max(0, midpoint - half)
end_time_s = midpoint + half
# Ensure minimum duration after clamping
if start_time_s == 0:
end_time_s = MIN_MOMENT_PREVIEW_DURATION_S| if duration < MIN_MOMENT_PREVIEW_DURATION_S: | |
| # Expand symmetrically around the midpoint to reach the minimum | |
| midpoint = (start_time_s + end_time_s) / 2 | |
| half = MIN_MOMENT_PREVIEW_DURATION_S / 2 | |
| start_time_s = max(0, midpoint - half) | |
| end_time_s = midpoint + half | |
| if duration < MIN_MOMENT_PREVIEW_DURATION_S: | |
| # Expand symmetrically around the midpoint to reach the minimum | |
| midpoint = (start_time_s + end_time_s) / 2 | |
| half = MIN_MOMENT_PREVIEW_DURATION_S / 2 | |
| start_time_s = max(0, midpoint - half) | |
| end_time_s = midpoint + half | |
| # Ensure minimum duration after clamping start to 0 | |
| if start_time_s == 0: | |
| end_time_s = MIN_MOMENT_PREVIEW_DURATION_S | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Claude speaking: Addressed — the fix from the suggested code block has been applied. After the symmetric expansion, there is now a check at lines 78–79: if start_time_s == 0 (i.e. the start was clamped), end_time_s is set to MIN_MOMENT_PREVIEW_DURATION_S to guarantee the full minimum duration. The parameterized test case clamp_start_extend_end (segment 5–10 s) now asserts expected_end == MIN_MOMENT_PREVIEW_DURATION_S and the new invariant assertion end_offset_s - start_offset_s >= MIN_MOMENT_PREVIEW_DURATION_S covers all cases.
Adds a SessionProblemSignalCard that displays: - Inline GIF preview of the problematic moment (from moment_preview_url) - Problem type badge (color-coded by severity) - Segment title, time range, user context - Source line label "Session replay · Session problem" Depends on PostHog/posthog#55463 for the backend moment preview rasterization.
Adds a SessionProblemSignalCard that displays: - Inline GIF preview of the problematic moment (from moment_preview_url) - Problem type badge (color-coded by severity) - Segment title, time range, user context - Source line label "Session replay · Session problem" Depends on PostHog/posthog#55463 for the backend moment preview rasterization.
Adds a SessionProblemSignalCard that displays: - Inline GIF preview of the problematic moment (from moment_preview_url) - Problem type badge (color-coded by severity) - Segment title, time range, user context - Source line label "Session replay · Session problem" Depends on PostHog/posthog#55463 for the backend moment preview rasterization.
Twixes
force-pushed
the
04-21-feat_signals_rasterize_session_problem_moments_as_gif_previews
branch
from
4a94e38 to
c9a1790
Compare
Adds a SessionProblemSignalCard that displays: - Inline GIF preview of the problematic moment (from moment_preview_url) - Problem type badge (color-coded by severity) - Segment title, time range, user context - Source line label "Session replay · Session problem" Depends on PostHog/posthog#55463 for the backend moment preview rasterization.
Adds a SessionProblemSignalCard that displays: - Inline GIF preview of the problematic moment (from moment_preview_url) - Problem type badge (color-coded by severity) - Segment title, time range, user context - Source line label "Session replay · Session problem" Depends on PostHog/posthog#55463 for the backend moment preview rasterization.
Adds a SessionProblemSignalCard that displays: - Inline GIF preview of the problematic moment (from moment_preview_url) - Problem type badge (color-coded by severity) - Segment title, time range, user context - Source line label "Session replay · Session problem" Depends on PostHog/posthog#55463 for the backend moment preview rasterization.
Adds a SessionProblemSignalCard that displays: - Inline GIF preview of the problematic moment (from moment_preview_url) - Problem type badge (color-coded by severity) - Segment title, time range, user context - Source line label "Session replay · Session problem" Depends on PostHog/posthog#55463 for the backend moment preview rasterization.
When a session problem signal is emitted, render a GIF of the problematic moment (minimum 30s) via the rasterize-recording Temporal workflow. The asset ID is stored in the signal extra and resolved to a public URL in the /signal_reports/.../signals/ API endpoint.
Twixes
force-pushed
the
04-21-feat_signals_rasterize_session_problem_moments_as_gif_previews
branch
from
c9a1790 to
9f0a561
Compare
1 participant
Problem
When session problem signals are emitted from the video summarization pipeline, there's no quick visual preview of what happened. Reviewers in PostHog Code have to open the full session replay to understand the problem.
Changes
emit_session_problem_signals_activityto rasterize a GIF preview of each problematic moment via therasterize-recordingTemporal workflowmoment_preview_asset_idin the signal'sextradict_enrich_signals_with_moment_preview_urls()in the signals API to batch-resolve asset IDs to JWT-signed public URLs at read timeHow did you test this code?
_classify_problem,_parse_timestamp_to_seconds, and_rasterize_moment_preview(happy path, short segment expansion, start clamping, failure cleanup)_enrich_signals_with_moment_preview_urls(URL resolution, no-op, missing asset)Publish to changelog?
No
🤖 LLM context
Co-authored with Claude Code.