feat: add URL playback support to play command#280
Conversation
Co-authored-by: ibigbug <543405+ibigbug@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds URL playback support to the play command, which previously only accepted local file paths (fixes #22). It also fixes cross-platform compilation issues exposed by correcting the Cargo.toml dependency gating for the windows crate.
Changes:
- Refactored
Sessioninplay.rsto use aBox<dyn BufRead>reader, enabling both file and HTTP response sources, withfrom_url()using the existingreqwest::blockingdependency andnormalize_url()to handle asciinema.org recording URLs - Fixed
Cargo.tomlto use proper[target.'cfg(windows)'.dependencies]syntax instead of a TOML comment that was mistaken for a Rust cfg attribute - Added
#[cfg(windows)]/#[cfg(not(windows))]guards throughoutrecord.rsandterminal/mod.rsto enable compilation on non-Windows platforms
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/commands/play.rs |
Core feature: abstracted Session reader to Box<dyn BufRead>, added from_url(), is_url(), normalize_url(), improved error handling, and added unit tests |
Cargo.toml |
Fixed windows crate dependency to be properly platform-gated |
src/commands/record.rs |
Added #[cfg(windows)] guards on WindowsTerminal import, struct field, and method calls; added non-Windows fallback with hardcoded dimensions and error exit |
src/terminal/mod.rs |
Scoped the test module to #[cfg(windows)] since it depends on WindowsTerminal |
src/main.rs |
Updated play subcommand help text to mention URL support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -23,7 +23,9 @@ use windows::Win32::{ | |||
|
|
|||
| use crate::commands::types::LineItem; | |||
| use crate::commands::types::RecordHeader; | |||
There was a problem hiding this comment.
The Terminal trait import is unconditional, but it is only used (indirectly) via WindowsTerminal which is gated behind #[cfg(windows)]. On non-Windows platforms this will produce an unused import warning. This import should also be gated with #[cfg(windows)].
| use crate::commands::types::RecordHeader; | |
| use crate::commands::types::RecordHeader; | |
| #[cfg(windows)] |
|
@copilot address comments and resolve conflicts |
…ze_url; gate Terminal import with cfg(windows) Co-authored-by: ibigbug <543405+ibigbug@users.noreply.github.com>
Addressed both review comments in b8ab76e:
|
|
@copilot there's still conflicts - make sure you resolve them properly |
…flicts Co-authored-by: ibigbug <543405+ibigbug@users.noreply.github.com>
Resolved in 0550d3e. The branch now includes all changes from main:
All 10 tests pass. |
|
@codex[agent] resolve PR conflicts |
Conflicts with main resolved and pushed in a15e75a. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude[agent] resolve conflicts and address pr commetns and update pr title and description proerlyh |
Resolved in e17583b:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot update PR title and body properly |
Updated the PR description with a proper summary of changes (closes #22, URL playback feature, v1 format support, cross-platform fixes). The title can't be updated programmatically with the permissions available — please manually rename it to something like: Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Adds URL playback support to the
playcommand, which previously only accepted local file paths. Also adds asciinema v1 format support and fixes cross-platform compilation issues.Changes
src/commands/play.rs):Sessionnow accepts aBox<dyn BufRead>reader, with a newfrom_url()constructor that fetches recordings over HTTP via the existingreqwest::blockingdependency.normalize_url()converts asciinema.org web URLs (e.g.https://asciinema.org/a/abc123) to their.castAPI endpoints, correctly handling query strings, URL fragments, and trailing slashes.src/commands/play.rs,src/commands/types.rs): Aparse_reader()helper auto-detects v1 (single JSON object withstdout) vs. v2 (header line + JSON event lines) formats.Cargo.toml,src/commands/record.rs,src/terminal/mod.rs): FixedCargo.tomlplatform-gating for thewindowscrate using proper[target.'cfg(windows)'.dependencies]syntax; added#[cfg(windows)]/#[cfg(not(windows))]guards throughoutrecord.rsandterminal/mod.rs.Usage
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.