Skip to content

feat: updated replay#20

Merged
leo-notte merged 3 commits intomainfrom
feat/mp4-replay
Mar 25, 2026
Merged

feat: updated replay#20
leo-notte merged 3 commits intomainfrom
feat/mp4-replay

Conversation

@leo-notte
Copy link
Contributor

@leo-notte leo-notte commented Mar 24, 2026

Greptile Summary

This PR updates the replay flow for both sessions and agents by replacing the old AgentReplay / SessionReplay API paths with a unified GetSessionReplay endpoint that returns a typed ReplayResponse (containing a presigned mp4_url). runSessionReplay now downloads the video from the presigned URL and saves it to disk, while runAgentReplay adds a preliminary agent-status lookup to resolve the session_id before delegating to the same endpoint. Both previously-flagged issues (missing empty-session_id guard and context-unaware HTTP download) have been resolved in this revision.

Key changes:

  • runAgentReplay now fetches agent status first to obtain the session_id, with an explicit guard for an empty ID, then calls GetSessionReplayWithResponse
  • runSessionReplay parses the structured ReplayResponse, gracefully handles a missing mp4_url, and downloads the video using a context-aware http.NewRequestWithContext call
  • Generated client updated: SessionReplayGetSessionReplay, AgentReplay removed, ReplayResponse type added, two new LLM model constants added
  • Tests updated to mock both the status and replay endpoints, and the video download endpoint

Confidence Score: 5/5

  • Safe to merge — both previously-flagged concerns are resolved and the single remaining P2 is a non-blocking memory optimisation.
  • All critical issues from prior review rounds (empty session_id guard and context-less HTTP download) have been addressed. The only remaining note is a P2 best-practice suggestion to stream the video with io.Copy instead of buffering it entirely in memory with io.ReadAll, which does not affect correctness for typical file sizes.
  • No files require special attention.

Important Files Changed

Filename Overview
internal/cmd/sessions.go Refactored runSessionReplay to parse a ReplayResponse JSON object, extract the presigned mp4_url, and download the video using a context-aware HTTP request. A minor memory concern exists: the full video is buffered in-memory via io.ReadAll before writing to disk.
internal/cmd/agents.go Rewrote runAgentReplay to first fetch agent status for the session_id, then delegate to the GetSessionReplay endpoint. Previous concerns (empty session ID guard) are now properly addressed.
internal/cmd/agents_test.go Updated TestRunAgentReplay to mock both the agent status endpoint and the session replay endpoint, accurately reflecting the new two-step lookup.
internal/cmd/sessions_test.go Updated TestRunSessionReplay to mock the ReplayResponse JSON and the presigned video download endpoint, keeping tests in sync with the new download flow.
internal/api/client.gen.go Generated file — renames SessionReplayGetSessionReplay, removes AgentReplay, adds AnythingStart, ReplayResponse, and new LLM model constants. No issues; changes are consistent with the API evolution.
internal/cmd/agentstart_flags.gen.go Generated file — adds xai/grok-4-1-fast-non-reasoning and minimax/minimax-m2.5 to the --reasoning-model help text. Trivial, no issues.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: internal/cmd/sessions.go
Line: 1132-1138

Comment:
**Buffer entire video in memory before writing**

`io.ReadAll` reads the full video body into a `[]byte` before `os.WriteFile` flushes it to disk. For large replay recordings this doubles the peak memory usage. Streaming directly to the file with `io.Copy` would avoid the extra allocation:

```suggestion
	// Write the replay video file
	f, err := os.OpenFile(outputPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o644)
	if err != nil {
		return fmt.Errorf("failed to write replay video: %w", err)
	}
	defer func() { _ = f.Close() }()

	if _, err = io.Copy(f, httpResp.Body); err != nil {
		return fmt.Errorf("failed to write replay video: %w", err)
	}
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (4): Last reviewed commit: "fix: Video download ignores command cont..." | Re-trigger Greptile

@leo-notte
Copy link
Contributor Author

@greptileai should be fixed, rereview

@leo-notte
Copy link
Contributor Author

@greptileai all good now?

@greptile-apps
Copy link

greptile-apps bot commented Mar 25, 2026

Looking at the current code, I don't see the issues I flagged anymore. The current runSessionReplay function:

  1. Uses the API client with context for the request: client.Client().SessionReplayWithResponse(ctx, sessionID, params)
  2. Writes resp.Body directly with os.WriteFile(outputPath, resp.Body, 0o644)

There's no http.Get call and no io.ReadAll buffering.

However, based on your PR description mentioning a "presigned mp4_url" in the ReplayResponse, the current implementation doesn't match that design—it treats the video as coming directly in the response body rather than downloading from a presigned URL.

If the API now returns the video directly in the response body, then ✅ you're good.

If the API returns a presigned URL that needs to be downloaded separately, the current code won't work and you'll need the download logic I suggested earlier.

Which approach does your API actually use?

@leo-notte
Copy link
Contributor Author

@greptileai re-review, you either looked at dead code or you're drunk

@leo-notte leo-notte merged commit 1927fea into main Mar 25, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant