Conversation
Removed CN release creation step and related environment variable.
Add a step to normalize the Cargo version in the workflow.
…into windows-arm
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[cfg(not(any(windows, target_os = "macos")))] | ||
| return Self::Windows; | ||
| } |
There was a problem hiding this comment.
Incorrect fallback platform for Linux/other OSes
The #[cfg(not(any(windows, target_os = "macos")))] branch returns Self::Windows for any non-Windows, non-macOS platform (e.g., Linux). This means Linux builds would silently report their platform as Windows, which is semantically wrong and could cause incorrect behaviour in any code that branches on the Platform value.
A Platform::Linux variant (or at minimum Platform::Unknown) should be used here. If Linux is not a supported target for the desktop app, this case should panic or return an error rather than silently lying.
| #[cfg(not(any(windows, target_os = "macos")))] | |
| return Self::Windows; | |
| } | |
| #[cfg(not(any(windows, target_os = "macos")))] | |
| return Self::Linux; |
Prompt To Fix With AI
This is a comment left during a code review. Path: crates/project/src/meta.rs Line: 61-63 Comment: **Incorrect fallback platform for Linux/other OSes** The `#[cfg(not(any(windows, target_os = "macos")))]` branch returns `Self::Windows` for any non-Windows, non-macOS platform (e.g., Linux). This means Linux builds would silently report their platform as `Windows`, which is semantically wrong and could cause incorrect behaviour in any code that branches on the `Platform` value. A `Platform::Linux` variant (or at minimum `Platform::Unknown`) should be used here. If Linux is not a supported target for the desktop app, this case should panic or return an error rather than silently lying. ```suggestion #[cfg(not(any(windows, target_os = "macos")))] return Self::Linux; ``` How can I resolve this? If you propose a fix, please make it concise.| @@ -414,416 +144,323 @@ impl ActorBuilder { | |||
| | |||
| pub async fn build( | |||
| self, | |||
| #[cfg(target_os = "macos")] shareable_content: cidre::arc::R<cidre::sc::ShareableContent>, | |||
| #[cfg(target_os = "macos")] _shareable_content: cidre::arc::R<cidre::sc::ShareableContent>, | |||
| ) -> anyhow::Result<ActorHandle> { | |||
There was a problem hiding this comment.
Camera feed silently dropped
self.camera_feed is captured in ActorBuilder but never forwarded to the underlying instant_recording::Actor::builder. The mic feed has an explicit forwarding block but the camera does not, so any recording that includes a camera will silently produce output with no camera track.
if let Some(mic_feed) = self.mic_feed { builder = builder.with_mic_feed(mic_feed); } // camera_feed is never passed — missing: if let Some(camera_feed) = self.camera_feed { builder = builder.with_camera_feed(camera_feed); }Prompt To Fix With AI
This is a comment left during a code review. Path: crates/recording/src/studio_recording.rs Line: 131-148 Comment: **Camera feed silently dropped** `self.camera_feed` is captured in `ActorBuilder` but never forwarded to the underlying `instant_recording::Actor::builder`. The mic feed has an explicit forwarding block but the camera does not, so any recording that includes a camera will silently produce output with no camera track. ```rust if let Some(mic_feed) = self.mic_feed { builder = builder.with_mic_feed(mic_feed); } // camera_feed is never passed — missing: if let Some(camera_feed) = self.camera_feed { builder = builder.with_camera_feed(camera_feed); } ``` How can I resolve this? If you propose a fix, please make it concise. apps/desktop/src-tauri/Cargo.toml Outdated
| [package] | ||
| name = "cap-desktop" | ||
| version = "0.3.83" | ||
| version = "0.4.92" |
There was a problem hiding this comment.
Version mismatch between Cargo.toml and Cargo.lock
Cargo.toml declares version = "0.4.92" but Cargo.lock records version = "0.4.87" for cap-desktop. These must match — Cargo uses the lock file to reproduce builds deterministically, and an inconsistent lock file can cause confusing build/resolution errors.
Run cargo update -p cap-desktop (or simply re-run the build) to regenerate Cargo.lock from Cargo.toml.
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src-tauri/Cargo.toml Line: 3 Comment: **Version mismatch between `Cargo.toml` and `Cargo.lock`** `Cargo.toml` declares `version = "0.4.92"` but `Cargo.lock` records `version = "0.4.87"` for `cap-desktop`. These must match — Cargo uses the lock file to reproduce builds deterministically, and an inconsistent lock file can cause confusing build/resolution errors. Run `cargo update -p cap-desktop` (or simply re-run the build) to regenerate `Cargo.lock` from `Cargo.toml`. How can I resolve this? If you propose a fix, please make it concise.| @@ -188,6 +177,25 @@ jobs: | |||
| if: ${{ env.RUN_BUILD == 'true' }} | |||
| uses: actions/checkout@v4 | |||
| | |||
| - name: Normalize Cargo version | |||
| if: ${{ env.RUN_BUILD == 'true' }} | |||
There was a problem hiding this comment.
All non-ARM artifact uploads have been removed
The original crabnebula-dev/cloud-release upload step ran for every build matrix entry (macOS arm64, macOS x64, Windows x64, Windows arm64). The replacement actions/upload-artifact@v4 step is conditioned on platform == 'windows' && arch == 'arm64' only. This means macOS and Windows x64 builds now produce no uploaded artifacts at all, which will break those release pipelines entirely.
Either restore the upload step for other platforms or add an else branch that handles the non-ARM case.
Prompt To Fix With AI
This is a comment left during a code review. Path: .github/workflows/publish.yml Line: 169-181 Comment: **All non-ARM artifact uploads have been removed** The original `crabnebula-dev/cloud-release` upload step ran for every build matrix entry (macOS arm64, macOS x64, Windows x64, Windows arm64). The replacement `actions/upload-artifact@v4` step is conditioned on `platform == 'windows' && arch == 'arm64'` only. This means macOS and Windows x64 builds now produce no uploaded artifacts at all, which will break those release pipelines entirely. Either restore the upload step for other platforms or add an `else` branch that handles the non-ARM case. How can I resolve this? If you propose a fix, please make it concise.![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let mut builder = instant_recording::Actor::builder( | ||
| self.output_path.clone(), | ||
| self.capture_target.clone(), | ||
| ) | ||
| .await | ||
| } | ||
| } | ||
| | ||
| #[tracing::instrument("studio_recording", skip_all)] | ||
| async fn spawn_studio_recording_actor( | ||
| recording_dir: PathBuf, | ||
| base_inputs: RecordingBaseInputs, | ||
| custom_cursor_capture: bool, | ||
| ) -> anyhow::Result<ActorHandle> { | ||
| ensure_dir(&recording_dir)?; | ||
| | ||
| trace!("creating recording actor"); | ||
| .with_system_audio(self.system_audio) | ||
| .with_max_output_size(self.max_fps); |
There was a problem hiding this comment.
with_max_output_size here looks like a unit mixup: max_output_size is a pixel constraint, but this field is FPS. With the current defaults this likely downscales output to ~60px.
| let mut builder = instant_recording::Actor::builder( | |
| self.output_path.clone(), | |
| self.capture_target.clone(), | |
| ) | |
| .await | |
| } | |
| } | |
| #[tracing::instrument("studio_recording", skip_all)] | |
| async fn spawn_studio_recording_actor( | |
| recording_dir: PathBuf, | |
| base_inputs: RecordingBaseInputs, | |
| custom_cursor_capture: bool, | |
| ) -> anyhow::Result<ActorHandle> { | |
| ensure_dir(&recording_dir)?; | |
| trace!("creating recording actor"); | |
| .with_system_audio(self.system_audio) | |
| .with_max_output_size(self.max_fps); | |
| let mut builder = instant_recording::Actor::builder( | |
| self.output_path.clone(), | |
| self.capture_target.clone(), | |
| ) | |
| .with_system_audio(self.system_audio); |
| | ||
| let segments_dir = ensure_dir(&content_dir.join("segments"))?; | ||
| let cursors_dir = ensure_dir(&content_dir.join("cursors"))?; | ||
| if let Some(mic_feed) = self.mic_feed { |
There was a problem hiding this comment.
camera_feed, custom_cursor, and fragmented are stored on ActorBuilder but never applied in build(). If studio-on-Windows intentionally falls back to instant recording, it might be clearer to remove these knobs (or plumb them through) so callers don’t assume they work.
| pub async fn resume(&self) -> anyhow::Result<()> { | ||
| Ok(self.actor_ref.ask(Resume).await?) | ||
| pub async fn is_paused(&self) -> anyhow::Result<bool> { | ||
| Ok(false) |
There was a problem hiding this comment.
is_paused() always returning false can cause surprising UI/state bugs (especially since pause()/resume() delegate to the inner actor). If the inner handle can’t report pause state, consider tracking it in ActorHandle (e.g., flip a flag in pause()/resume()).
| cursor: self.cursor, | ||
| Ok(CompletedRecording { | ||
| project_path: self.project_path.clone(), | ||
| meta: default_studio_meta(), |
There was a problem hiding this comment.
stop() currently returns default_studio_meta() regardless of what was actually recorded (system audio/mic/camera/cursor, fps, etc). If downstream uses this meta to render/edit, this could be a silent correctness issue; even a minimal meta derived from the builder inputs would be safer than a hard-coded default.
| let display = Video::new(s.display.path.to_path(recording_path), 0.0) | ||
| .expect("Failed to read display video"); | ||
| let camera = s.camera.as_ref().map(|camera| { | ||
| Video::new(camera.path.to_path(recording_path), 0.0) | ||
| .expect("Failed to read camera video") | ||
| }); | ||
| let mic = s | ||
| .audio | ||
| .as_ref() | ||
| .map(|audio| Audio::new(audio.path.to_path(recording_path), 0.0)) | ||
| .transpose() | ||
| .expect("Failed to read audio"); |
There was a problem hiding this comment.
Minor: these expect()s will panic the whole app on a malformed/incomplete recording directory. Since ProjectRecordingsMeta::new already returns Result, it’s easy to propagate a useful error instead.
| let display = Video::new(s.display.path.to_path(recording_path), 0.0) | |
| .expect("Failed to read display video"); | |
| let camera = s.camera.as_ref().map(|camera| { | |
| Video::new(camera.path.to_path(recording_path), 0.0) | |
| .expect("Failed to read camera video") | |
| }); | |
| let mic = s | |
| .audio | |
| .as_ref() | |
| .map(|audio| Audio::new(audio.path.to_path(recording_path), 0.0)) | |
| .transpose() | |
| .expect("Failed to read audio"); | |
| let display = Video::new(s.display.path.to_path(recording_path), 0.0) | |
| .map_err(|e| format!("display / {e}"))?; | |
| let camera = s | |
| .camera | |
| .as_ref() | |
| .map(|camera| Video::new(camera.path.to_path(recording_path), 0.0)) | |
| .transpose() | |
| .map_err(|e| format!("camera / {e}"))?; | |
| let mic = s | |
| .audio | |
| .as_ref() | |
| .map(|audio| Audio::new(audio.path.to_path(recording_path), 0.0)) | |
| .transpose() | |
| .map_err(|e| format!("mic / {e}"))?; |
| return Self::MacOS; | ||
| | ||
| #[cfg(not(any(windows, target_os = "macos")))] | ||
| return Self::Windows; |
There was a problem hiding this comment.
This makes non-Windows/non-macOS builds report Platform::Windows, which seems like it could silently mis-label recordings (or enable Windows-only behavior) if someone builds tooling on Linux.
| return Self::Windows; | |
| #[cfg(not(any(windows, target_os = "macos")))] | |
| compile_error!("Unsupported platform"); |
There was a problem hiding this comment.
Pull request overview
This PR aims to get the desktop app building/working on Windows ARM by adjusting the recording implementation and the release workflow, plus a few related fixes/formatting updates.
Changes:
- Reworks
studio_recordingto delegate to theinstant_recordingactor and adds new builder knobs (fragmented,max_fps). - Adjusts the publish workflow to build Windows ARM and upload the ARM installer artifact; removes CrabNebula release steps.
- Adds a new Solid app entry (
apps/desktop/src/app.tsx) and makes a few smaller tweaks (Rust duration parsing, typings formatting, platform default).
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-solid/src/auto-imports.d.ts | Formatting/consistency tweak in generated icon type imports. |
| crates/rendering/src/project_recordings.rs | Refactors stream metadata access to avoid repeated borrows while deriving duration. |
| crates/recording/src/studio_recording.rs | Major rewrite: studio recording now wraps instant recording; introduces metadata parsing types. |
| crates/project/src/meta.rs | Adds a non-Windows/non-macOS fallback for Platform::default(). |
| apps/desktop/src/app.tsx | New app/root wiring with router, query client, toaster, and theme listener. |
| apps/desktop/src-tauri/src/recording.rs | Uses new studio builder options (with_fragmented, with_max_fps). |
| apps/desktop/src-tauri/Cargo.toml | Bumps cap-desktop version. |
| Cargo.lock | Updates lockfile entry for cap-desktop version. |
| .github/workflows/publish.yml | Workflow changes for Windows ARM, signing/updater config prep, artifact upload changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .expect("Failed to read display video"); | ||
| let camera = s.camera.as_ref().map(|camera| { | ||
| Video::new(camera.path.to_path(recording_path), 0.0) | ||
| .expect("Failed to read camera video") | ||
| }); | ||
| let mic = s | ||
| .audio | ||
| .as_ref() | ||
| .map(|audio| Audio::new(audio.path.to_path(recording_path), 0.0)) | ||
| .transpose() | ||
| .expect("Failed to read audio"); |
There was a problem hiding this comment.
ProjectRecordingsMeta::new returns Result<_, String> but uses expect(...) when loading the display/camera videos, which will panic the process instead of returning an error. Convert these to error propagation (e.g., map_err + ?) so callers get a Err(String) rather than a crash.
| .expect("Failed to read display video"); | |
| let camera = s.camera.as_ref().map(|camera| { | |
| Video::new(camera.path.to_path(recording_path), 0.0) | |
| .expect("Failed to read camera video") | |
| }); | |
| let mic = s | |
| .audio | |
| .as_ref() | |
| .map(|audio| Audio::new(audio.path.to_path(recording_path), 0.0)) | |
| .transpose() | |
| .expect("Failed to read audio"); | |
| .map_err(|e| format!("segment 0 / video / {e}"))?; | |
| let camera = s | |
| .camera | |
| .as_ref() | |
| .map(|camera| { | |
| Video::new(camera.path.to_path(recording_path), 0.0) | |
| .map_err(|e| format!("segment 0 / camera / {e}")) | |
| }) | |
| .transpose()?; | |
| let mic = s | |
| .audio | |
| .as_ref() | |
| .map(|audio| { | |
| Audio::new(audio.path.to_path(recording_path), 0.0) | |
| .map_err(|e| format!("segment 0 / mic / {e}")) | |
| }) | |
| .transpose()?; |
| #[derive(Debug, Clone, Copy, Serialize, Type)] | ||
| pub struct Video { | ||
| pub duration: f64, | ||
| pub width: u32, | ||
| pub height: u32, | ||
| pub fps: u32, | ||
| pub start_time: f64, | ||
| } |
There was a problem hiding this comment.
This file reintroduces Video/Audio/ProjectRecordingsMeta/SegmentRecordings logic that already exists in crates/rendering/src/project_recordings.rs (very similar code and structs). Duplicating this duration/metadata parsing increases the risk of behavior drift and platform-specific bugs; consider moving these types into a shared module/crate or reusing the rendering implementation instead of maintaining two copies.
| if let Some(mic_feed) = self.mic_feed { | ||
| builder = builder.with_mic_feed(mic_feed); | ||
| } | ||
| | ||
| let start_time = Timestamps::now(); | ||
| #[cfg(target_os = "macos")] |
There was a problem hiding this comment.
ActorBuilder exposes camera_feed, custom_cursor, and fragmented, but build() never applies any of these to the spawned actor (only system_audio and mic_feed are forwarded). This makes the corresponding builder methods effectively no-ops and will silently drop requested camera/cursor/fragmented behavior. Either wire these options through (likely by restoring/using the studio pipeline) or remove the options from the API to avoid misleading callers.
| | ||
| #[cfg(not(any(windows, target_os = "macos")))] | ||
| return Self::Windows; |
There was a problem hiding this comment.
For non-Windows and non-macOS targets, Platform::default() now returns Platform::Windows. That will misreport the platform on Linux/other builds and may leak into persisted metadata. If other platforms are supported, consider adding an explicit Linux/Unknown variant (or returning None at call sites) rather than defaulting to Windows.
| pub async fn stop(&self) -> anyhow::Result<CompletedRecording> { | ||
| let _ = self.inner.stop().await?; | ||
| | ||
| Ok(FinishedPipeline { | ||
| start_time: self.start_time, | ||
| screen: screen.context("screen")?, | ||
| microphone: microphone.transpose().context("microphone")?, | ||
| camera: camera.transpose().context("camera")?, | ||
| system_audio: system_audio.transpose().context("system_audio")?, | ||
| cursor: self.cursor, | ||
| Ok(CompletedRecording { | ||
| project_path: self.project_path.clone(), |
There was a problem hiding this comment.
ActorHandle::stop() discards the result of self.inner.stop() and returns a hard-coded default_studio_meta() instead. This will lose real capture metadata (e.g., actual fps/sample rate/output paths) and can make downstream logic think the project has different assets than were actually recorded. Consider mapping the inner completed recording info into a correct StudioRecordingMeta (or returning Instant meta if studio now delegates to instant).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if: ${{ env.RUN_BUILD == 'true' }} | ||
| uses: actions/checkout@v4 | ||
| | ||
| - name: Normalize Cargo version |
There was a problem hiding this comment.
Minor workflow gotcha: the draft job reads apps/desktop/src-tauri/Cargo.toml and creates the tag before this runs. So if the version ever accidentally becomes cap-vX, this step would normalize only for the build while the tag would still become cap-vcap-vX.
I’d either move this normalization into draft (before Read version number) or make this step fail fast instead of mutating the file.
| process.env.GITHUB_ENV, | ||
| `TAURI_SIGNING_PRIVATE_KEY<<__TAURI_KEY__\n${key}\n__TAURI_KEY__\n`, | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
If the signing key is missing/invalid this silently disables updater + artifacts, which can be pretty confusing to debug from logs. Consider emitting a warning here.
| } else { | |
| } else { | |
| console.warn("TAURI_SIGNING_PRIVATE_KEY missing/invalid; disabling updater and updater artifacts"); |
| - name: Upload Windows ARM installer artifact | ||
| if: ${{ env.RUN_BUILD == 'true' && matrix.settings.platform == 'windows' && matrix.settings.arch == 'arm64' }} | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| working-directory: apps/desktop | ||
| command: release upload ${{ env.CN_APPLICATION }} "${{ needs.draft.outputs.version }}" --framework tauri | ||
| api-key: ${{ secrets.CN_API_KEY }} | ||
| env: | ||
| TAURI_BUNDLE_PATH: ../.. | ||
| name: windows-arm-installer | ||
| path: target/${{ matrix.settings.target }}/release/bundle/nsis/*.exe | ||
| if-no-files-found: error |
There was a problem hiding this comment.
Right now this only uploads an artifact for Windows ARM, so the other matrix builds (macOS, Windows x64) end up with no uploaded bundle at all. If the goal is just to add ARM support, it’d be nice to keep artifacts for the other targets too.
| - name: Upload Windows ARM installer artifact | |
| if: ${{ env.RUN_BUILD == 'true' && matrix.settings.platform == 'windows' && matrix.settings.arch == 'arm64' }} | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| working-directory: apps/desktop | |
| command: release upload ${{ env.CN_APPLICATION }} "${{ needs.draft.outputs.version }}" --framework tauri | |
| api-key: ${{ secrets.CN_API_KEY }} | |
| env: | |
| TAURI_BUNDLE_PATH: ../.. | |
| name: windows-arm-installer | |
| path: target/${{ matrix.settings.target }}/release/bundle/nsis/*.exe | |
| if-no-files-found: error | |
| - name: Upload Tauri bundle artifacts | |
| if: ${{ env.RUN_BUILD == 'true' }} | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: tauri-${{ matrix.settings.platform }}-${{ matrix.settings.arch }} | |
| path: | | |
| target/${{ matrix.settings.target }}/release/bundle/nsis/*.exe | |
| target/${{ matrix.settings.target }}/release/bundle/dmg/*.dmg | |
| if-no-files-found: error |
| const key = [...candidates].find( | ||
| (value) => | ||
| value.includes("untrusted comment:") && | ||
| value.toLowerCase().includes("minisign secret key"), | ||
| ); |
There was a problem hiding this comment.
Minor robustness: this untrusted comment: check is case-sensitive, while the next check lowercases. Making both case-insensitive avoids unexpectedly disabling updater artifacts if the secret formatting changes slightly.
| const key = [...candidates].find( | |
| (value) => | |
| value.includes("untrusted comment:") && | |
| value.toLowerCase().includes("minisign secret key"), | |
| ); | |
| const key = [...candidates].find((value) => { | |
| const lower = value.toLowerCase(); | |
| return lower.includes("untrusted comment:") && lower.includes("minisign secret key"); | |
| }); |
| if: ${{ env.RUN_BUILD == 'true' }} | ||
| uses: crabnebula-dev/cloud-release@v0 | ||
| - name: Upload Windows ARM installer artifact | ||
| if: ${{ env.RUN_BUILD == 'true' && matrix.settings.platform == 'windows' && matrix.settings.arch == 'arm64' }} |
There was a problem hiding this comment.
This upload step is gated to Windows ARM only, so Windows x64 (and macOS) builds won't upload any artifacts anymore. If the goal is just to add ARM support, consider uploading for all Windows matrix entries (and adding similar uploads for macOS), or restoring the previous release upload.
| if: ${{ env.RUN_BUILD == 'true' && matrix.settings.platform == 'windows' && matrix.settings.arch == 'arm64' }} | |
| - name: Upload Windows installer artifact | |
| if: ${{ env.RUN_BUILD == 'true' && matrix.settings.platform == 'windows' }} | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: windows-${{ matrix.settings.arch }}-installer | |
| path: target/${{ matrix.settings.target }}/release/bundle/nsis/*.exe | |
| if-no-files-found: error |
| if: ${{ env.RUN_BUILD == 'true' }} | ||
| uses: crabnebula-dev/cloud-release@v0 | ||
| - name: Upload Windows ARM installer artifact | ||
| if: ${{ env.RUN_BUILD == 'true' && matrix.settings.platform == 'windows' && matrix.settings.arch == 'arm64' }} |
There was a problem hiding this comment.
This upload step only runs for Windows ARM64, so the other matrix entries won’t publish any artifacts. If the intent is to keep artifacts for every build, consider uploading per target/arch with a dynamic name.
| if: ${{ env.RUN_BUILD == 'true' && matrix.settings.platform == 'windows' && matrix.settings.arch == 'arm64' }} | |
| - name: Upload build artifacts | |
| if: ${{ env.RUN_BUILD == 'true' }} | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: ${{ matrix.settings.platform }}-${{ matrix.settings.arch }}-bundle | |
| path: target/${{ matrix.settings.target }}/release/bundle/** | |
| if-no-files-found: error |
| .with_system_audio(self.system_audio) | ||
| .with_max_fps(self.max_fps); |
There was a problem hiding this comment.
instant_recording::ActorBuilder doesn’t define with_max_fps (at least in this PR), so this won’t compile. If this is meant to cap FPS, it probably needs to be threaded into ScreenCaptureConfig::init; otherwise drop the call for now.
| .with_system_audio(self.system_audio) | |
| .with_max_fps(self.max_fps); | |
| .with_system_audio(self.system_audio); |
| cursor: self.cursor, | ||
| Ok(CompletedRecording { | ||
| project_path: self.project_path.clone(), | ||
| meta: default_studio_meta(), |
There was a problem hiding this comment.
stop() throws away the CompletedRecording returned by instant_recording and returns hardcoded studio metadata. That’s very likely to drift from what was actually produced (paths, fps, audio presence, etc.) and will break anything that consumes StudioRecordingMeta. I’d derive StudioRecordingMeta from the inner CompletedRecording (or return the instant meta directly if studio is now just a wrapper).
| return Self::MacOS; | ||
| | ||
| #[cfg(not(any(windows, target_os = "macos")))] | ||
| return Self::Windows; |
There was a problem hiding this comment.
Defaulting to Windows on non-Windows/non-macOS targets will silently mislabel metadata. If Linux/other targets aren’t supported, I’d rather make this loud than lie.
| return Self::Windows; | |
| return unreachable!("unsupported platform"); |
/!\ This is mostly vibe-coded
I needed the windows arm version because the emulation layer wasn't enough good to make it work
I let an IA to fix enough to make it work
If this can help you
You can also find the binary in the artecfacts of my github action : https://github.com/Volko61/CapWindowsArm/actions/runs/23456210577/artifacts/6067435266
I just hope the maintainer can use my changes to really make a published version of the windows arm
Greptile Summary
This PR adds Windows on ARM (aarch64-pc-windows-msvc) support by redirecting the studio recording path through the existing
instant_recordingactor and updating the CI workflow to build and upload a Windows ARM installer artifact. While the motivation is sound, several correctness issues were introduced in the process that would prevent the app from working reliably even on the target platform.Key issues found:
crates/recording/src/studio_recording.rsreplaces the full recording actor with a stub that silently drops the camera feed, always returnsis_paused() == false, and returns hardcoded metadata (content/output.mp4, fps 30) that does not match the actual files produced byinstant_recording. Any attempt to open or render the completed recording will fail.meta.rs: The new#[cfg(not(any(windows, target_os = "macos")))]branch returnsPlatform::Windowsfor Linux builds instead of a correct Linux or unknown value.upload-artifactstep means macOS and Windows x64 builds no longer upload any artifacts, breaking those release pipelines.Cargo.lockout of sync:Cargo.tomldeclares0.4.92butCargo.lockrecords0.4.87; the lock file needs to be regenerated.rendering/src/project_recordings.rsrefactor and therecording.rs.with_fragmented(false).with_max_fps(60)additions are clean and safe.Confidence Score: 1/5
is_paused()is a no-op, the platform enum has a wrong fallback, artifact uploads are broken for non-ARM targets, and Cargo.lock is out of sync. These are not cosmetic issues — they affect the primary recording user path and CI for all platforms.crates/recording/src/studio_recording.rsis the most critical file and needs a complete correctness review..github/workflows/publish.ymlneeds the artifact upload regression fixed before merging.Important Files Changed
windows-latest-ltowindows-latestfor both Windows targets.instant_recording. Camera feed is silently dropped,is_paused()always returns false, andstop()returns hardcoded stub metadata that won't match actual output files.Platform::Windowsfor non-Windows, non-macOS platforms (e.g. Linux).0.3.83to0.4.92, butCargo.lockrecords0.4.87— the lock file was not regenerated to match..with_fragmented(false).with_max_fps(60)to the screen capture builder and removes the unusedselfimport — straightforward and safe changes.Video::newto pre-cache stream metadata and avoid repeated borrow-after-move issues when iterating packets. Logic is equivalent to the original; the change is a safe cleanup.App.tsxtoapp.tsxto match the kebab-case file naming convention. No content changes.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[ActorBuilder::build] --> B[instant_recording::Actor::builder] B --> C[with_system_audio] C --> D{mic_feed?} D -- Yes --> E[with_mic_feed] D -- No --> F[⚠️ camera_feed silently dropped] E --> F F --> G[builder.build → inner ActorHandle] G --> H[ActorHandle wraps inner] H --> I[stop] H --> J[pause / resume / cancel] H --> K[is_paused → always false ⚠️] I --> L[inner.stop] L --> M[CompletedRecording] M --> N[default_studio_meta — hardcoded stub ⚠️] N --> O[path: content/output.mp4\nfps: 30\nstart_time: None]Comments Outside Diff (3)
crates/recording/src/studio_recording.rs, line 645-648 (link)is_paused()always returnsfalseThis method unconditionally returns
Ok(false)regardless of whether the recording is actually paused. Any UI or logic that queries pause state will never see a paused status, making pause/resume effectively invisible to callers.Prompt To Fix With AI
crates/recording/src/studio_recording.rs, line 551-566 (link)stop()returns hardcoded stub metadata that won't match actual recorded filesdefault_studio_meta()returns aStudioRecordingMeta::SingleSegmentwith a hardcoded path (content/output.mp4) and hardcoded fps (30). The underlyinginstant_recordingactor produces files at a different path/layout, so the returnedCompletedRecordingmetadata will not point to any real files. Any downstream code that opens these paths will fail.The metadata should be derived from the actual output of
self.inner.stop()rather than substituted with a placeholder.Prompt To Fix With AI
.github/workflows/publish.yml, line 59-63 (link)windows-latestrunner may not support native ARM64 buildsGitHub-hosted
windows-latestrunners are x64. Cross-compilingaarch64-pc-windows-msvcfrom x64 is possible with the MSVC toolchain, but some native build steps (e.g., running.exepost-build hooks, certain LLVM/bindgen invocations) can fail or produce unexpected results. If a dedicated Windows ARM runner is available in the org, it should be preferred. Otherwise, ensure the cross-compilation path has been explicitly tested end-to-end.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "dza" | Re-trigger Greptile
(2/5) Greptile learns from your feedback when you react with thumbs up/down!