feat: Screenshot editor overhaul - layers panel, keyboard shortcuts, and sharper rendering #1432
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Layers panel with drag‑and‑drop reorder/history, expands screenshot‑editor context (prettyName, popover control, focus), refines annotation resize logic and UI panels, broadens camera/pixel‑format and audio/sample handling, updates rendering sampling/AA, and enables Windows MSVC in CI plus small UI/tooling tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LayersPanel
participant EditorCtx as ScreenshotContext
participant Anns as AnnotationsState
participant Hist as ProjectHistory
User->>LayersPanel: click layer item
LayersPanel->>EditorCtx: setFocusAnnotationId(id)
LayersPanel->>EditorCtx: setTool("select")
EditorCtx->>Anns: mark focused annotation
User->>LayersPanel: mousedown grip (start drag)
LayersPanel->>LayersPanel: startDrag(dragState)
User->>LayersPanel: mousemove (update)
LayersPanel->>LayersPanel: compute dropTargetIndex (visual)
User->>LayersPanel: mouseup (drop)
LayersPanel->>Anns: reorderAnnotations(newOrder)
Anns->>Hist: push history entry
LayersPanel->>LayersPanel: clear dragState / update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts (1)
294-294: Consider sanitizing the filename.
prettyNameis used directly as the default filename. IfprettyNamecontains characters invalid for filenames (e.g.,/,\,:,*,?,",<,>,|), the save dialog may behave unexpectedly or fail.- defaultPath: `${prettyName}.png`, + defaultPath: `${prettyName.replace(/[/\\:*?"<>|]/g, "_")}.png`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.github/workflows/ci.yml(1 hunks).vscode/extensions.json(1 hunks)apps/desktop/src-tauri/src/screenshot_editor.rs(4 hunks)apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsx(1 hunks)apps/desktop/src/routes/editor/Player.tsx(0 hunks)apps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsx(4 hunks)apps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsx(2 hunks)apps/desktop/src/routes/screenshot-editor/AnnotationTools.tsx(1 hunks)apps/desktop/src/routes/screenshot-editor/Editor.tsx(11 hunks)apps/desktop/src/routes/screenshot-editor/Header.tsx(4 hunks)apps/desktop/src/routes/screenshot-editor/LayersPanel.tsx(1 hunks)apps/desktop/src/routes/screenshot-editor/PresetsDropdown.tsx(0 hunks)apps/desktop/src/routes/screenshot-editor/Preview.tsx(7 hunks)apps/desktop/src/routes/screenshot-editor/context.tsx(5 hunks)apps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsx(2 hunks)apps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsx(1 hunks)apps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsx(1 hunks)apps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsx(1 hunks)apps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsx(1 hunks)apps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts(2 hunks)apps/desktop/src/routes/target-select-overlay.tsx(3 hunks)apps/desktop/src/utils/tauri.ts(1 hunks)crates/rendering/src/composite_frame.rs(1 hunks)crates/rendering/src/lib.rs(1 hunks)crates/rendering/src/shaders/composite-video-frame.wgsl(2 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/routes/editor/Player.tsx
- apps/desktop/src/routes/screenshot-editor/PresetsDropdown.tsx
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsxapps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsxcrates/rendering/src/lib.rscrates/rendering/src/composite_frame.rsapps/desktop/src/utils/tauri.tsapps/desktop/src-tauri/src/screenshot_editor.rsapps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsxapps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsxapps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsxapps/desktop/src/routes/screenshot-editor/LayersPanel.tsxapps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsxapps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsxapps/desktop/src/routes/screenshot-editor/context.tsxapps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsxapps/desktop/src/routes/screenshot-editor/Header.tsxapps/desktop/src/routes/screenshot-editor/AnnotationTools.tsxapps/desktop/src/routes/screenshot-editor/Preview.tsxapps/desktop/src/routes/target-select-overlay.tsx
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsxapps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsxapps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsxapps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsxapps/desktop/src/routes/screenshot-editor/LayersPanel.tsxapps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsxapps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsxapps/desktop/src/routes/screenshot-editor/context.tsxapps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsxapps/desktop/src/routes/screenshot-editor/Header.tsxapps/desktop/src/routes/screenshot-editor/AnnotationTools.tsxapps/desktop/src/routes/screenshot-editor/Preview.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsxapps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsxapps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsxapps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsxapps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsxapps/desktop/src/routes/screenshot-editor/LayersPanel.tsxapps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsxapps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsxapps/desktop/src/routes/screenshot-editor/context.tsxapps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsxapps/desktop/src/routes/screenshot-editor/Header.tsxapps/desktop/src/routes/screenshot-editor/AnnotationTools.tsxapps/desktop/src/routes/screenshot-editor/Preview.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsxapps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsxapps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsxapps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsxapps/desktop/src/routes/screenshot-editor/LayersPanel.tsxapps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsxapps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsxapps/desktop/src/routes/screenshot-editor/context.tsxapps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsxapps/desktop/src/routes/screenshot-editor/Header.tsxapps/desktop/src/routes/screenshot-editor/AnnotationTools.tsxapps/desktop/src/routes/screenshot-editor/Preview.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsxapps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsxapps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsxapps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsxapps/desktop/src/routes/screenshot-editor/LayersPanel.tsxapps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsxapps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsxapps/desktop/src/routes/screenshot-editor/context.tsxapps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsxapps/desktop/src/routes/screenshot-editor/Header.tsxapps/desktop/src/routes/screenshot-editor/AnnotationTools.tsxapps/desktop/src/routes/screenshot-editor/Preview.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsxapps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsxcrates/rendering/src/lib.rscrates/rendering/src/composite_frame.rsapps/desktop/src/utils/tauri.tsapps/desktop/src-tauri/src/screenshot_editor.rsapps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsxapps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsxapps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsxapps/desktop/src/routes/screenshot-editor/LayersPanel.tsxapps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsxapps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsxapps/desktop/src/routes/screenshot-editor/context.tsxapps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsxapps/desktop/src/routes/screenshot-editor/Header.tsxapps/desktop/src/routes/screenshot-editor/AnnotationTools.tsxapps/desktop/src/routes/screenshot-editor/Preview.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
crates/rendering/src/lib.rscrates/rendering/src/composite_frame.rsapps/desktop/src-tauri/src/screenshot_editor.rs
apps/desktop/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated files: tauri.ts, queries.ts, and files under apps/desktop/src-tauri/gen/
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts
{**/tauri.ts,**/queries.ts,apps/desktop/src-tauri/gen/**}
📄 CodeRabbit inference engine (AGENTS.md)
Never edit auto-generated files including
**/tauri.ts,**/queries.ts, andapps/desktop/src-tauri/gen/**
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/screenshot_editor.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx,solid} : Never manually import desktop app icons; use unplugin-icons auto-import instead
Applied to files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
📚 Learning: 2025-11-29T04:31:23.896Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: Turborepo monorepo structure: `apps/desktop` (Tauri v2 + SolidStart), `apps/web` (Next.js), `apps/cli` (Rust CLI), `packages/*` for shared libs, `crates/*` for Rust crates
Applied to files:
.vscode/extensions.json
📚 Learning: 2025-11-19T18:25:41.457Z
Learnt from: ItsEeleeya
Repo: CapSoftware/Cap PR: 1396
File: apps/desktop/src/routes/(window-chrome)/settings/general.tsx:183-192
Timestamp: 2025-11-19T18:25:41.457Z
Learning: In SolidJS, when using the Show component with the `keyed` attribute and a dynamic reactive value (like a memo result), the component will recreate its children when the condition value changes by reference. This pattern is useful for triggering CSS animations (like fade-in) when the rendered content needs to update. For example, in apps/desktop/src/routes/(window-chrome)/settings/general.tsx, `<Show when={previews()[theme.id]} keyed>` ensures the image re-animates when switching between different preview sources.
Applied to files:
apps/desktop/src/routes/screenshot-editor/Preview.tsx
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/web/**/*.tsx : Minimize useEffect usage: compute during render, handle logic in event handlers, and ensure cleanups for subscriptions/timers
Applied to files:
apps/desktop/src/routes/screenshot-editor/Preview.tsx
🧬 Code graph analysis (10)
apps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsx (1)
apps/desktop/src/routes/screenshot-editor/ui.tsx (1)
EditorButton(353-420)
apps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsx (1)
apps/desktop/src/routes/screenshot-editor/ui.tsx (1)
EditorButton(353-420)
apps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts (1)
apps/desktop/src/routes/screenshot-editor/context.tsx (1)
prettyName(316-318)
apps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsx (1)
apps/desktop/src/routes/screenshot-editor/ui.tsx (1)
EditorButton(353-420)
apps/desktop/src/routes/screenshot-editor/Editor.tsx (3)
apps/desktop/src/routes/screenshot-editor/Header.tsx (1)
Header(33-178)apps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsx (1)
AnnotationConfigBar(9-175)apps/desktop/src/routes/screenshot-editor/LayersPanel.tsx (1)
LayersPanel(29-295)
apps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsx (3)
apps/desktop/src/routes/screenshot-editor/ui.tsx (1)
Slider(65-147)apps/desktop/src/routes/screenshot-editor/ColorPicker.tsx (2)
RgbInput(24-83)rgbToHex(85-90)apps/desktop/src/components/Tooltip.tsx (1)
Tooltip(23-47)
apps/desktop/src/routes/screenshot-editor/context.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
XY(505-505)
apps/desktop/src/routes/screenshot-editor/Header.tsx (2)
apps/desktop/src/routes/screenshot-editor/context.tsx (1)
path(313-315)apps/desktop/src-tauri/src/screenshot_editor.rs (1)
remove(361-371)
apps/desktop/src/routes/screenshot-editor/AnnotationTools.tsx (1)
apps/desktop/src/components/Tooltip.tsx (1)
Tooltip(23-47)
apps/desktop/src/routes/target-select-overlay.tsx (1)
crates/cursor-capture/src/position.rs (1)
crop(180-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (42)
apps/desktop/src/routes/(window-chrome)/new-main/TargetMenuGrid.tsx (1)
88-97: ViewAllButton spacing adjustment is safe and consistentAdding
mb-3tightens vertical spacing control for the bottom “View all” row without affecting behavior, accessibility, or layout semantics. Looks good as a purely presentational tweak..vscode/extensions.json (1)
2-2: Clarify the scope of the VS Code extensions change.This PR replaces
tauri-apps.tauri-vscodewithbiomejs.biome, but the PR objectives focus on screenshot editor UI/UX and rendering fixes with no mention of a tooling migration. Biome (a code formatter/linter) and tauri-vscode (a Tauri language extension) serve different purposes.Please clarify: Is this change intentional as part of a broader tooling migration, or was it unintended? If intentional, it should be documented in the PR description and coordinated across the team.
apps/desktop/src/routes/target-select-overlay.tsx (4)
70-70: LGTM: Appropriate minimum size for screenshot mode.The 1×1 minimum allows captures of any size in screenshot mode, which is appropriate since screenshots don't have the same encoding constraints as video recordings.
435-436: LGTM: Clean mode-aware minimum size selector.The function correctly returns the appropriate minimum based on the recording mode. SolidJS reactivity will properly track
options.modeas a dependency in both usage locations.
438-442: LGTM: Validation now mode-aware.The validation logic correctly adapts to screenshot versus video recording modes. The memo will properly track
options.modechanges through theminSize()call.
789-791: LGTM: Clear, mode-specific user feedback.The dynamic error message provides appropriate feedback based on the current mode. Users will see "Minimum size is 1 x 1" in screenshot mode and "Minimum size is 150 x 150" in video modes.
.github/workflows/ci.yml (1)
106-107: LGTM!Enabling Clippy for the Windows MSVC target is a good addition for cross-platform code quality. This aligns with the existing
build-desktopandrust-cachejobs that already include Windows targets.apps/desktop/src-tauri/src/screenshot_editor.rs (2)
33-34: LGTM!The
pretty_namefield is properly added toScreenshotEditorInstanceand correctly initialized fromrecording_meta.pretty_nameat line 268.
380-381: LGTM!The
pretty_namefield is correctly added to the serialized struct with proper#[serde(rename_all = "camelCase")]applied, ensuring it serializes asprettyNamefor the TypeScript frontend.apps/desktop/src/routes/screenshot-editor/AnnotationLayer.tsx (2)
246-262: LGTM!The drawing constraint logic correctly differentiates between:
- Circles: default to square proportions (shift releases the constraint)
- Rectangles/masks: shift constrains to square
- Arrows: shift snaps to 45° increments
Sign preservation for drag direction is handled correctly.
336-355: LGTM!The resize constraint logic is well-structured with clear separation between circle and rectangle constraints. The origin recalculation for west/north handles correctly adjusts the position when constraining dimensions.
apps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts (1)
11-19: LGTM!Clean destructuring of
prettyNamefrom the context for use in the export flow.apps/desktop/src/routes/screenshot-editor/popovers/RoundingPopover.tsx (1)
24-32: LGTM!Switching to controlled popover state via
activePopoverandsetActivePopoverfrom context ensures consistent behavior across all editor popovers—only one popover can be open at a time.apps/desktop/src/routes/screenshot-editor/popovers/BackgroundSettingsPopover.tsx (1)
125-131: LGTM!The controlled popover pattern using
activePopovercontext state is correctly implemented. The keyboard shortcut hintkbd={["B"]}for "Background" is appropriately added to the trigger.Also applies to: 203-212
apps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsx (1)
18-27: LGTM!The controlled popover pattern is correctly implemented, consistent with other popovers in the codebase.
apps/desktop/src/utils/tauri.ts (1)
1-3: Auto-generated file - no review required.This file is generated by tauri-specta and should not be manually edited. The type changes reflect backend modifications.
apps/desktop/src/routes/screenshot-editor/popovers/BorderPopover.tsx (1)
13-26: LGTM!The controlled popover pattern with
activePopovercontext state is correctly implemented, consistent with other popovers. The keyboard shortcutkbd={["E"]}is appropriately added.apps/desktop/src/routes/screenshot-editor/AnnotationTools.tsx (1)
17-31: LGTM!The Layers panel toggle button is well-implemented with consistent styling patterns matching the existing
ToolButtoncomponent. The keyboard shortcut hintkbd={["L"]}is appropriately added.apps/desktop/src/routes/screenshot-editor/popovers/PaddingPopover.tsx (1)
7-14: LGTM!The controlled popover pattern is correctly implemented, consistent with other popovers in the codebase.
apps/desktop/src/routes/screenshot-editor/Preview.tsx (4)
42-50: LGTM!The
zoomInandzoomOuthelper functions are cleanly extracted, properly clamping zoom values and resetting pan on zoom changes.
52-76: LGTM!The keyboard shortcut handler is well-implemented:
- Properly guards against input elements and contentEditable
- Correctly uses
onCleanupfor listener removal- Handles both
metaKey(macOS) andctrlKey(Windows/Linux) modifiers
318-345: LGTM!The focus annotation effect correctly uses SolidJS's
on()helper for explicit dependency tracking. The coordinate transformation logic properly calculates the pan offset to center the focused annotation in the viewport, and immediately clears the focus state to prevent re-triggering.
140-162: LGTM!The zoom button refactoring to use the extracted helpers is clean, and the
kbdhints correctly indicate the keyboard shortcuts.apps/desktop/src/routes/screenshot-editor/Editor.tsx (3)
109-125: New keyboard shortcuts are well-implemented.The toggle pattern for popovers (
activePopover() === "X" ? null : "X") is consistent and correct. Thelkey for layers panel uses the dedicated setter which is appropriate.One consideration: the
hkey for "shadow" andekey for "border" are somewhat non-intuitive mnemonics. If this was intentional (e.g., avoiding conflicts), it's fine, but consider documenting these shortcuts in the UI or help section for discoverability.
214-228: LGTM!The
initialBoundsderivation correctly handles both cases: when an existing crop is present and when starting fresh. The fallback to fulloriginalSizedimensions is appropriate for the initial state.
136-146: LGTM!The layout structure is clean. Wrapping Header and AnnotationConfigBar in a relative container allows for proper absolute positioning of the config bar. The conditional LayersPanel rendering using
Showis idiomatic SolidJS.apps/desktop/src/routes/screenshot-editor/context.tsx (3)
113-123: LGTM!Good separation of concerns:
layersPanelOpenis correctly persisted (user preference that should survive sessions)focusAnnotationIdandactivePopoverare correctly non-persisted (ephemeral UI state)The
activePopoverunion type is well-defined and covers all popover variants.
28-32: LGTM!The
cropdialog type correctly captures the required state:originalSizefor the source dimensions and nullablecurrentCropfor the existing crop bounds. This aligns with the Header.tsxcropDialogHandlerimplementation.
316-318: LGTM!The
prettyNamegetter follows the same pattern aspathand provides a sensible fallback for export naming.apps/desktop/src/routes/screenshot-editor/AnnotationConfig.tsx (3)
34-35: LGTM!Converting
typeandisMaskto function accessors is the correct SolidJS pattern. This ensures reactivity is preserved when the annotation type changes.
177-193: LGTM!
ConfigItemis a clean helper component that reduces duplication across the config bar. The optionalvalueprop pattern works well for displaying current values alongside controls.
210-227: Verify popover state management consistency.Other popovers in the editor (background, padding, shadow, border) integrate with
activePopovercontext to ensure only one popover is open at a time. ThisColorPickerButtonuses its own independent Popover state.If multiple ColorPickerButtons or other popovers can be open simultaneously, this may cause UI clutter. Consider whether this popover should also integrate with
activePopoverfor mutual exclusion, or if the current behavior (independent color picker popovers) is intentional.apps/desktop/src/routes/screenshot-editor/LayersPanel.tsx (4)
115-131: LGTM!The index calculation correctly handles the reversed display order. The compensation for
targetIdx > draggedReversedIdxaccounts for the index shift that occurs when splicing out the dragged item before reinserting it.
152-159: LGTM!The delete handler correctly:
- Prevents event bubbling
- Records history before mutation (enabling undo)
- Removes the annotation
- Clears selection if the deleted item was selected
161-169: LGTM!This effect is a good defensive measure. It ensures drag state is reset when the annotations array changes (e.g., due to external deletion or undo/redo), preventing stale references.
200-278: LGTM!The layer item rendering is well-structured:
- Derived signals (
isSelected,isDragging,isDropTarget,showDropIndicatorAfter) correctly compute reactive state- Drop indicator logic properly avoids showing indicators adjacent to the dragged item
- The grip handle pattern with
data-grip-handleattribute is a clean approach for distinguishing drag vs click targetsapps/desktop/src/routes/screenshot-editor/Header.tsx (2)
34-36: LGTM! Path accessor refactored correctly.The change from a direct property to a function accessor aligns with the context API update where
pathis now a getter. The usage at lines 147 and 160 correctly invokespath()to retrieve the file path.
40-65: LGTM! Keyboard shortcuts implemented correctly.The keyboard shortcut handler properly guards against input fields and contentEditable elements, uses platform-appropriate modifier keys, and includes proper cleanup. The logic prevents duplicate exports by checking
isExporting()state.crates/rendering/src/lib.rs (1)
1215-1224: Display border fallback now matches transparent defaultUsing
[0.0, 0.0, 0.0, 0.0]forborder_colorwhenproject.background.borderisNoneor disabled keeps the display path consistent with the new default uniforms and with the camera/camera-only paths, eliminating the previous implicit white edge.crates/rendering/src/shaders/composite-video-frame.wgsl (2)
222-261: Crop-aware UV clamping and sharpen scaling look correctThe updated
sample_texturelogic:
- Maps
uvinto the crop rectangle viacrop_sizeandcrop_bounds_uv.xy.- Clamps
cropped_uvtosafe_min/safe_maxusing a one-texel margin derived from1.0 / uniforms.frame_size, which should prevent sampling past the texture edge while still covering the full visible crop.- Derives
source_sizeasframe_size * crop_size, so downscale detection andsharpnessare now based on the actual cropped content rather than the full frame, which matches the new crop behavior.This is consistent with the CPU uniforms (crop_bounds in pixels, normalized to
crop_bounds_uv) and should address edge artifacts without introducing sampling errors.Please sanity-check with very small crop regions to ensure your UI’s minimum crop size always keeps the crop at least a couple of texels wide/high so the safe_min/safe_max clamping behaves as intended across all devices.
269-278: Rounded-corner AA change should reduce jaggies while preserving interior opacityUsing
anti_alias_width = max(fwidth(distance), 0.5)andcoverage = 1.0 - smoothstep(0.0, anti_alias_width, distance)keeps pixels with negative SDF (inside the rounded rect) fully opaque and only fades out asdistancemoves from 0 to positive values outside the shape. The larger minimum AA width should smooth high-res edges and avoid the previous “jagged” look at corners.It’s worth validating this visually on both small and large targets to ensure the 0.5 minimum width gives the desired softness across resolutions and doesn’t overly blur very thin borders.
crates/rendering/src/composite_frame.rs (1)
61-64: Default border color set to transparent black is safe and prevents unintended renderingThe change to
border_color: [0.0, 0.0, 0.0, 0.0]inCompositeVideoFrameUniforms::defaultis safe because the shader only uses this value whenborder_enabled > 0.0. The default disables borders (border_enabled: 0.0), so the border color is never rendered unless explicitly enabled elsewhere in the code. All call sites usingDefault::default()immediately override the uniforms, and all explicit initializations setborder_enabledintentionally.
apps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsx
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 8
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/routes/screenshot-editor/Header.tsx(5 hunks)apps/desktop/src/routes/screenshot-editor/LayersPanel.tsx(1 hunks)apps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsx(2 hunks)scripts/setup.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/routes/screenshot-editor/LayersPanel.tsx
- apps/desktop/src/routes/screenshot-editor/popovers/ShadowPopover.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
scripts/setup.jsapps/desktop/src/routes/screenshot-editor/Header.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
scripts/setup.jsapps/desktop/src/routes/screenshot-editor/Header.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
scripts/setup.jsapps/desktop/src/routes/screenshot-editor/Header.tsx
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/screenshot-editor/Header.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/screenshot-editor/Header.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/screenshot-editor/Header.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
🧬 Code graph analysis (2)
scripts/setup.js (1)
apps/desktop/scripts/prodBeforeBundle.js (1)
targetDir(14-14)
apps/desktop/src/routes/screenshot-editor/Header.tsx (2)
apps/desktop/src/routes/screenshot-editor/context.tsx (1)
path(313-315)apps/desktop/src-tauri/src/screenshot_editor.rs (1)
remove(361-371)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
apps/desktop/src/routes/screenshot-editor/Header.tsx (3)
34-36: LGTM: Clean path accessor refactor.The function-based
path()accessor aligns with the context pattern and properly handles missing instances via optional chaining.
67-79: LGTM: Guard correctly prevents invalid crop dialog.The
frame?.dataguard ensures the dialog only opens with valid dimensions, andisCropDisabled()provides consistent button state. This directly resolves the critical issue flagged in the past review (commit 7d5371a).
94-94: LGTM: Proper button state management.Disabling the crop button when frame data is unavailable provides clear feedback and prevents user confusion.
scripts/setup.js (2)
111-119: Windowstarextraction change with--force-locallooks correctAdding
--force-localto thetarinvocation is a good fix for Windows paths containing drive letters (e.g.,C:\...), preventing them from being interpreted as remote archives and avoiding sporadic extraction failures. The rest of the extraction flow (execFileargs order, subsequentrmandrename) remains unchanged and consistent with the previous logic.Please verify this on your target Windows environments (local + CI) to confirm
tarsupports--force-localeverywhere you expect this script to run.
227-245: Usingentry.parentPathfor codesign targets is the right Dirent APISwitching to
path.join(entry.parentPath, entry.name)aligns with the modernfs.DirentAPI, wherepathis documented as a deprecated alias forparentPath. This keeps behavior the same while avoiding deprecated properties and correctly handles recursivereaddirresults by constructing a clear absolute path to each.dylib. (docs.deno.com)Ensure your Node runtime (both locally and in CI) is recent enough to support
fs.readdirwith{ recursive: true, withFileTypes: true }andDirent.parentPathso this continues to work across all macOS build machines.
| <DropdownItem | ||
| onSelect={() => { | ||
| revealItemInDir(path); | ||
| revealItemInDir(path()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's read the Header.tsx file to understand the context around lines 149 and 162
echo "=== Reading Header.tsx around lines 149 and 162 ==="
wc -l apps/desktop/src/routes/screenshot-editor/Header.tsxRepository: CapSoftware/Cap
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Get context around lines 149 and 162
sed -n '140,170p' apps/desktop/src/routes/screenshot-editor/Header.tsxRepository: CapSoftware/Cap
Length of output: 945
🏁 Script executed:
#!/bin/bash
# Find the full Header.tsx file to see more context
sed -n '1,50p' apps/desktop/src/routes/screenshot-editor/Header.tsxRepository: CapSoftware/Cap
Length of output: 2058
🏁 Script executed:
#!/bin/bash
# Look for isCropDisabled pattern mentioned in review
echo "=== Searching for isCropDisabled guard ==="
rg -n "isCropDisabled" apps/desktop/src/routes/screenshot-editor -C3Repository: CapSoftware/Cap
Length of output: 1181
🏁 Script executed:
#!/bin/bash
# Search for editorInstance initialization and nullability patterns
echo "=== Searching for editorInstance initialization ==="
rg -n "editorInstance" apps/desktop/src/routes/screenshot-editor -C5 | head -60Repository: CapSoftware/Cap
Length of output: 4634
🏁 Script executed:
#!/bin/bash
# Check if there are any disabled state checks on dropdown items
echo "=== Checking DropdownItem component definition ==="
rg -n "DropdownItem" apps/desktop/src/routes/screenshot-editor/ui -C5Repository: CapSoftware/Cap
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Look for how buttons are disabled based on path or editorInstance
echo "=== Searching for disabled patterns on buttons/items ==="
rg -n "disabled.*path|path.*disabled" apps/desktop/src/routes/screenshot-editorRepository: CapSoftware/Cap
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Find DropdownItem component definition
echo "=== Searching for DropdownItem component ==="
find apps/desktop/src -name "*.tsx" -o -name "*.ts" | xargs rg -l "DropdownItem" | head -10Repository: CapSoftware/Cap
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Find the ui directory/file where DropdownItem is defined
echo "=== Locating DropdownItem export ==="
rg -n "export.*DropdownItem|function DropdownItem|const DropdownItem" apps/desktop/src/routes/screenshot-editorRepository: CapSoftware/Cap
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Search more broadly for DropdownItem definition
echo "=== Broad search for DropdownItem definition ==="
rg -n "DropdownItem.*=" apps/desktop/src --type=tsx --type=tsRepository: CapSoftware/Cap
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Check the import statement in Header.tsx more carefully
echo "=== Checking where DropdownItem is imported from ==="
head -40 apps/desktop/src/routes/screenshot-editor/Header.tsx | grep -A2 -B2 "DropdownItem"Repository: CapSoftware/Cap
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Read the DropdownItem implementation
echo "=== Reading DropdownItem implementation ==="
sed -n '275,295p' apps/desktop/src/routes/screenshot-editor/ui.tsxRepository: CapSoftware/Cap
Length of output: 659
🏁 Script executed:
#!/bin/bash
# Read more of the DropdownItem implementation
echo "=== Full DropdownItem implementation ==="
sed -n '281,300p' apps/desktop/src/routes/screenshot-editor/ui.tsxRepository: CapSoftware/Cap
Length of output: 641
🏁 Script executed:
#!/bin/bash
# Check full context of Header.tsx to see if dropdown is disabled at higher level
echo "=== Reading full Header.tsx dropdown context ==="
sed -n '130,180p' apps/desktop/src/routes/screenshot-editor/Header.tsxRepository: CapSoftware/Cap
Length of output: 1502
🏁 Script executed:
#!/bin/bash
# Search for any guards on path usage in screenshot editor
echo "=== Searching for path validation/guards ==="
rg -n 'path\(\)' apps/desktop/src/routes/screenshot-editor -B2 -A2Repository: CapSoftware/Cap
Length of output: 1747
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern for checking editorInstance loading state elsewhere
echo "=== Checking for editorInstance loading state checks ==="
rg -n "editorInstance().*pending|loading|error" apps/desktop/src/routes/screenshot-editorRepository: CapSoftware/Cap
Length of output: 625
🏁 Script executed:
#!/bin/bash
# See if path is checked anywhere else before use
echo "=== Checking if path is validated before critical operations ==="
rg -n 'if.*path|path.*if' apps/desktop/src/routes/screenshot-editor -C2Repository: CapSoftware/Cap
Length of output: 561
Add guards to prevent Open Folder and Delete actions when path is empty.
The path() accessor returns "" when editorInstance() hasn't loaded. The Open Folder and Delete dropdown items execute without guards, unlike the Crop button which uses isCropDisabled(). Add a guard function (e.g., isPathAvailable() => path() !== "") and apply it to both DropdownItem elements with a disabled prop to prevent unintended behavior, especially for the remove("") call.
🤖 Prompt for AI Agents
In apps/desktop/src/routes/screenshot-editor/Header.tsx around line 149, the
Open Folder and Delete DropdownItem handlers call revealItemInDir(path()) and
remove(path()) even when path() === "" (editorInstance not loaded); add a small
guard function isPathAvailable = () => path() !== "" and use it to set the
disabled prop on both DropdownItem elements and gate their onClick handlers so
they no-op when false (prevent calling revealItemInDir("") or remove("")).
Ensure the Crop button's existing isCropDisabled pattern is followed for
consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/setup.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
scripts/setup.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
scripts/setup.js
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
scripts/setup.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
🧬 Code graph analysis (1)
scripts/setup.js (1)
apps/desktop/scripts/prodBeforeBundle.js (2)
exec(9-9)targetDir(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
| await exec( | ||
| `Expand-Archive -Path "${ffmpegZipPath}" -DestinationPath "${targetDir}" -Force`, | ||
| { shell: "powershell.exe" }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 150 scripts/setup.js | tail -n 50Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
cat -n scripts/setup.js | sed -n '100,130p'Repository: CapSoftware/Cap
Length of output: 1568
🏁 Script executed:
cat package.json | jq '.engines'Repository: CapSoftware/Cap
Length of output: 79
🏁 Script executed:
rg -n "engines|node.*version" package.json apps/desktop/package.jsonRepository: CapSoftware/Cap
Length of output: 177
🏁 Script executed:
head -n 30 scripts/setup.jsRepository: CapSoftware/Cap
Length of output: 962
🏁 Script executed:
cat -n scripts/setup.js | sed -n '1,20p'Repository: CapSoftware/Cap
Length of output: 819
🏁 Script executed:
rg -n "catch|try|error" scripts/setup.js | head -20Repository: CapSoftware/Cap
Length of output: 397
Expand-Archive approach is appropriate for the Node.js 20+ environment, but consider state consistency in failure scenarios.
The PowerShell Expand-Archive approach is suitable for Windows environments given the Node.js >=20 requirement. However, the extraction sequence has a potential state inconsistency: if Expand-Archive succeeds but the subsequent fs.rename() fails (line 118), the old ffmpegDir has already been deleted (line 117), leaving the system without a valid ffmpeg directory. The exec() call lacks error handling, so failures will correctly propagate and stop the build, but recovery after partial completion isn't possible.
PowerShell availability is not a concern for supported environments—powershell.exe is standard on Windows 10+ and aligns with the Node.js >=20 requirement.
🤖 Prompt for AI Agents
In scripts/setup.js around lines 113 to 116, the PowerShell Expand-Archive call
can leave the repo without a valid ffmpeg directory if rename/delete steps later
fail; change the extraction to a safe two-phase approach: extract into a
temporary directory (e.g., targetDir + ".tmp" or a unique temp path), perform
any validations, then atomically rename the temp dir to the final ffmpegDir and
only then remove the previous ffmpegDir; wrap the exec/rename/remove sequence in
try/catch to rollback (delete the temp dir) on error and ensure the old
ffmpegDir is not deleted until the new one is successfully in place.
| exec( | ||
| `codesign ${keychain} -s "${signId}" -f "${path.join( | ||
| entry.parentPath || entry.path, | ||
| entry.parentPath, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat package.json | grep -A 5 '"engines"'Repository: CapSoftware/Cap
Length of output: 94
🏁 Script executed:
if [ -f apps/desktop/package.json ]; then cat apps/desktop/package.json | grep -A 5 '"engines"'; fiRepository: CapSoftware/Cap
Length of output: 41
Update Node.js engine requirement to >=20.1.0 or use fallback for entry.parentPath.
The project requires Node.js >=20 (package.json), but entry.parentPath was added in Node.js v20.1.0. Code using entry.parentPath will fail at runtime if Node.js 20.0.0 is installed. Either update the engines field to "node": ">=20.1.0" or use a fallback pattern like entry.parentPath || path.dirname(entry.path).
🤖 Prompt for AI Agents
In scripts/setup.js around line 237, the code uses entry.parentPath which only
exists in Node >=20.1.0; update package.json engines to "node": ">=20.1.0" OR
change the usage to a safe fallback such as using entry.parentPath ||
path.dirname(entry.path) (ensure path is imported) so the code works on Node
20.0.0 and above; pick one approach and apply consistently where
entry.parentPath is referenced.
| exec( | ||
| `codesign ${keychain} -s "${signId}" -f "${path.join( | ||
| entry.parentPath || entry.path, | ||
| entry.parentPath, |
There was a problem hiding this comment.
Bug: Node.js backward compatibility removed for Dirent property
The change from entry.parentPath || entry.path to just entry.parentPath removes backward compatibility with Node.js versions before v21.4.0. The parentPath property was added in Node.js v21.4.0, while path was the previous property name (now deprecated). On older Node.js versions, entry.parentPath will be undefined, causing the codesign path to fail silently or throw an error when constructing the file path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/windows.rs (1)
725-732: Verify fake-window listener integration for the in-progress recording windowReusing
fake_window::spawn_fake_window_listenerhere is consistent with theRecordingsOverlaypath and should give you the same cursor pass-through behavior. The correctness hinges onFakeWindowBoundsbeing populated for this window’s label; otherwise, the listener will keepignore_cursor_eventstrue and the countdown window may become fully click-through (no clickable cancel/controls).Also keep in mind this adds another long-lived 20 Hz task without explicit shutdown, same as the existing overlay usage.
If you see any issues in manual testing, consider:
- Ensuring bounds are registered for this label wherever
FakeWindowBoundsis managed.- Optionally adding a termination condition to
spawn_fake_window_listenertied to the window’s close lifecycle to avoid extra background loops over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/routes/in-progress-recording.tsx(3 hunks)crates/audio/src/latency.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src/routes/in-progress-recording.tsxapps/desktop/src-tauri/src/windows.rscrates/audio/src/latency.rs
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/in-progress-recording.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
apps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src/routes/in-progress-recording.tsxapps/desktop/src-tauri/src/windows.rscrates/audio/src/latency.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
apps/desktop/src-tauri/src/windows.rscrates/audio/src/latency.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/windows.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Applied to files:
apps/desktop/src-tauri/src/windows.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/windows.rs (1)
apps/desktop/src-tauri/src/fake_window.rs (2)
spawn_fake_window_listener(48-106)app(52-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
crates/audio/src/latency.rs (1)
440-448: Clamp refactor is correct and consistent with existing behaviorSwitching the non‑macOS fallback to use
.clamp(FALLBACK_WIRED_LATENCY_SECS, MAX_LATENCY_SECS)preserves the previousmax(...).min(...)semantics while matching the rest of the module’s style. For the tested 48000/512 case it still yields 0.03, so existing tests and behavior remain valid.apps/desktop/src/routes/in-progress-recording.tsx (3)
554-563: Excellent visual feedback and accessibility improvements.The transition effects, hover/active states, and aria-label enhance both the user experience and accessibility. The disabled state correctly prevents hover effects while maintaining visual feedback through opacity.
730-733: LGTM! Smooth hover feedback for the drag handle.The transition and theme-aware hover states provide clear visual feedback that this area is interactive, with consistent timing matching other UI elements.
743-759: Well-designed interactive states.The ActionButton component now has polished, consistent visual feedback across all states. The progressive opacity for hover/active states, theme-aware colors, and proper disabled state handling (including cursor changes) demonstrate attention to UX detail.
apps/desktop/src-tauri/src/windows.rs (1)
703-718: Explicitly disabling shadow for the countdown overlay looks consistent with other overlaysSetting
.shadow(false)here matches the other transparent, always-on-top overlay windows and should avoid odd-looking drop shadows around the in-progress recording bubble. This looks correct as long as design wants a completely flush overlay across platforms.Please sanity-check the visual result on macOS and Windows to confirm there are no regressions in how the countdown bubble blends with the screen.
crates/frame-converter/src/d3d11.rs
Outdated
| match pixel { | ||
| Pixel::NV12 => Ok(DXGI_FORMAT_NV12), | ||
| Pixel::YUYV422 => Ok(DXGI_FORMAT_YUY2), | ||
| Pixel::UYVY422 => Ok(DXGI_FORMAT_YUY2), |
There was a problem hiding this comment.
Bug: UYVY422 incorrectly mapped to YUY2 DXGI format
Mapping Pixel::UYVY422 to DXGI_FORMAT_YUY2 is incorrect because these formats have different byte orders. YUY2 stores data as Y-U-Y-V while UYVY stores data as U-Y-V-Y. When UYVY422 frame data is copied into a texture tagged as YUY2, the D3D11 Video Processor will misinterpret the U and V channels, resulting in incorrect colors in the output. DXGI doesn't have a native UYVY format, so UYVY422 input should either be pre-converted or should return an unsupported format error.
crates/media-info/src/lib.rs
Outdated
| SampleFormat::I32 => Some(Sample::I32(Type::Planar)), | ||
| SampleFormat::U32 => Some(Sample::I32(Type::Planar)), | ||
| SampleFormat::I64 => Some(Sample::I64(Type::Planar)), | ||
| SampleFormat::U64 => Some(Sample::I64(Type::Planar)), |
There was a problem hiding this comment.
Bug: Audio sample format mapping causes data misinterpretation
The new audio sample format mappings incorrectly map signed formats to unsigned and vice versa without converting the actual sample values. SampleFormat::I8 is mapped to Sample::U8, SampleFormat::U16 to Sample::I16, SampleFormat::U32 to Sample::I32, and SampleFormat::U64 to Sample::I64. Since the raw audio bytes are copied directly without value conversion, this causes the data to be misinterpreted. For example, I8 silence (value 0) becomes U8 minimum amplitude, and U16 values get their sign bit reinterpreted as negative values. This will cause significant audio distortion including DC offset and waveform corruption.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/scap-targets/src/platform/win.rs (1)
734-763: Executable-name filtering is broken and inconsistentThe filtering logic for
IGNORED_EXESis ineffective and uses inconsistent approaches across functions:
File extension mismatch in equality checks (lines ~1118, ~1162)
IGNORED_EXEScontains bare names:"webview2","msedgewebview2","cap"exe_namefromfile_name()includes the extension:"cap.exe","webview2.exe", etc.- After
to_lowercase(), comparisons likeIGNORED_EXES.contains("cap.exe")fail to match"cap"in the list- Result: WebView2/Edge WebView2/Cap processes are not being filtered as intended
Overly broad substring match in
is_window_valid_for_topmost_selection(line ~1189)
- Uses
exe_name_lower.contains("cap")instead of checking the list- Excludes any executable containing those three letters, not just the intended processes
Inconsistent filtering semantics
- Functions use equality checks against
IGNORED_EXES- Another function uses hard-coded substring checks
- Difficult to reason about which windows are actually excluded
Fix by using
file_stem()instead offile_name()to strip extensions, or add.exetoIGNORED_EXES. Better yet, centralize the logic into a single helper function to avoid repeatedto_lowercase()allocations and keep filtering behavior in one place.
♻️ Duplicate comments (2)
crates/media-info/src/lib.rs (1)
318-332: Critical: Incorrect audio sample format mappings cause data corruption.The mappings at lines 320, 323, 325, and 327 map between signed and unsigned formats without converting sample values. Since
wrap_frameand related methods perform direct byte copies (see lines 159, 170, 184), the raw audio data is reinterpreted with incorrect signedness:
I8(signed) →U8(unsigned): silence (0) becomes minimum amplitudeU16(unsigned) →I16(signed): values ≥32768 become negativeU32→I32,U64→I64: similar sign misinterpretationThis produces severe audio distortion, DC offset, and waveform corruption.
Return
Nonefor unsupported formats instead:pub fn ffmpeg_sample_format_for(sample_format: SampleFormat) -> Option<Sample> { match sample_format { - SampleFormat::I8 => Some(Sample::U8(Type::Planar)), SampleFormat::U8 => Some(Sample::U8(Type::Planar)), SampleFormat::I16 => Some(Sample::I16(Type::Planar)), - SampleFormat::U16 => Some(Sample::I16(Type::Planar)), SampleFormat::I32 => Some(Sample::I32(Type::Planar)), - SampleFormat::U32 => Some(Sample::I32(Type::Planar)), SampleFormat::I64 => Some(Sample::I64(Type::Planar)), - SampleFormat::U64 => Some(Sample::I64(Type::Planar)), SampleFormat::F32 => Some(Sample::F32(Type::Planar)), SampleFormat::F64 => Some(Sample::F64(Type::Planar)), _ => None, } }crates/frame-converter/src/d3d11.rs (1)
498-505: Do not mapPixel::UYVY422toDXGI_FORMAT_YUY2without converting the byte layout
DXGI_FORMAT_YUY2describes a YUY2 layout (Y0 U0 Y1 V0), while UYVY422 frames are laid out as U0 Y0 V0 Y1. With the new mapping:
pixel_to_dxgi(Pixel::UYVY422)returnsDXGI_FORMAT_YUY2, andcopy_frame_to_mappedjust memcpy’s the UYVY bytes into a YUY2-tagged texture.- The D3D11 video processor will then interpret the data as YUY2, so luma/chroma bytes are in the wrong positions, producing incorrect colors.
- Additionally,
copy_mapped_to_framehas noPixel::UYVY422arm; ifoutput_formatis everPixel::UYVY422, the output frame’s data will not be populated at all.Unless you add an explicit UYVY↔YUY2 swizzle in the copy paths, this mapping is unsound. The safest minimal fix is to keep UYVY422 unsupported in this backend and let the existing CPU path handle it:
fn pixel_to_dxgi(pixel: Pixel) -> Result<DXGI_FORMAT, ConvertError> { match pixel { Pixel::NV12 => Ok(DXGI_FORMAT_NV12), Pixel::YUYV422 => Ok(DXGI_FORMAT_YUY2), - Pixel::UYVY422 => Ok(DXGI_FORMAT_YUY2), _ => Err(ConvertError::UnsupportedFormat(pixel, Pixel::NV12)), } }If you do need GPU acceleration for UYVY422, you’ll want separate match arms and a per-pixel swizzle in
copy_frame_to_mapped(and the reverse incopy_mapped_to_frame) instead of treating UYVY422 as layout-compatible with YUY2.What is the exact byte layout of DXGI_FORMAT_YUY2 in Direct3D 11, and how does it differ from the UYVY (UYVY422) pixel format used by cameras/FFmpeg?
🧹 Nitpick comments (7)
apps/desktop/src/routes/in-progress-recording.tsx (2)
555-563: Stop button hover/active/disabled styling improvement; consider disabled cursor semanticsThe new
transition-colors+ hover/active backgrounds give clearer feedback and the disabled hover fix is good. Consider also addingdisabled:cursor-not-allowedhere (as inActionButton) for consistency and clearer “non-interactive” affordance when the button is disabled, unless you intentionally want the default cursor.
731-735: Drag handle hover transition improves affordance;hover:cursor-movemay be redundantThe hover background and
transition-colorsmake the drag affordance clearer. Givencursor-moveis already set on the base element,hover:cursor-moveis likely redundant unless overridden bynon-styled-move; if not needed, it can be dropped to simplify the class list.apps/desktop/src-tauri/src/camera.rs (1)
46-48: Consider accessor methods to preserve validation.Making these fields public bypasses the
clamp_size()validation logic thatget_state()andset_state()enforce. External code could now setsizeto values outsideMIN_CAMERA_SIZE..MAX_CAMERA_SIZEdirectly.Since
recording.rsonly reads theshapefield, consider:
- Adding a
pub fn shape(&self) -> &CameraPreviewShapeaccessor instead of exposing all fields- Or document that external code must respect the validation contract when directly accessing fields
-pub struct CameraPreviewState { - pub size: f32, - pub shape: CameraPreviewShape, - pub mirrored: bool, -} +pub struct CameraPreviewState { + size: f32, + shape: CameraPreviewShape, + mirrored: bool, +} + +impl CameraPreviewState { + pub fn shape(&self) -> &CameraPreviewShape { + &self.shape + } + + pub fn size(&self) -> f32 { + self.size + } + + pub fn mirrored(&self) -> bool { + self.mirrored + } +}crates/recording/src/sources/microphone.rs (1)
244-276: Consider adding tests for the new sample formats.The test suite currently only covers F32 format. Adding tests for the newly supported formats (I8, U16, U32, U64) would help ensure correctness and prevent regressions.
crates/camera-ffmpeg/src/windows.rs (1)
17-39: MJPEG decoding implementation looks correct but has performance implications.The decoder is created fresh for each frame. For high frame rates, this could add overhead. If performance becomes an issue, consider caching the decoder context. However, this is a reasonable initial implementation that prioritizes correctness.
crates/frame-converter/src/videotoolbox.rs (1)
235-269: Single‑plane CVPixelBuffer handling is correct; CVPixelBufferIsPlanar could be a minor clarity winBranching on
plane_count == 0to detect non‑planar CVPixelBuffers and usingCVPixelBufferGetBaseAddress/GetBytesPerRow/GetHeightin that case lines up with how non‑planar buffers are meant to be accessed (and with existing practice in CoreVideo bridge code whereCVPixelBufferGetPlaneCountreturns 0 for non‑planar formats). (chromium.googlesource.com)The row‑by‑row copy using
copy_len = src_stride.min(dst_stride)mirrors the existing multi‑plane path and standard VideoToolbox/FFmpeg interop patterns, so the new single‑plane case should behave consistently across BGRA/ARGB and packed YUYV/UYVY outputs. (ffmpeg.org)As an optional readability tweak (no behavior change), you could branch on
CVPixelBufferIsPlanar(pixel_buffer)instead ofplane_count == 0, treating the non‑planar branch as “single pseudo‑plane” and the planar branch as the existing per‑plane loop. That would make the intent (“planar vs non‑planar”) more explicit without changing logic.If you do consider switching to
CVPixelBufferIsPlanar, double‑check on your supported OS versions that its behavior for all of the pixel formats you care about (422, NV12, BGRA/ARGB) matches the currentGetPlaneCountcontract.crates/scap-targets/src/platform/win.rs (1)
538-548: Icon extraction order and early exits: behavior change is reasonable, but avoid repeated exe-path lookupsThe new
app_iconflow (shell icon → high‑res executable icons → window big icon → legacy ExtractIconExW path → small/window/class icons) is a clear improvement: you prefer higher‑quality icons and exit early once a good candidate is found. Resource cleanup still happens inside the helper functions, so no obvious leaks.One thing to tweak:
self.get_executable_path()is now called up to three times in this function, each time opening the process and querying its full path. Caching the path once per call toapp_iconand reusing it across branches would avoid redundant Win32 calls without hurting readability.You could, for example, compute an
exe_pathOption<String>once at the top ofapp_iconand share it across the shell‑icon, high‑res executable, and legacy ExtractIconExW branches.Also applies to: 558-563, 616-627
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/desktop/src-tauri/src/audio_meter.rs(1 hunks)apps/desktop/src-tauri/src/camera.rs(1 hunks)apps/desktop/src-tauri/src/recording.rs(3 hunks)apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/routes/in-progress-recording.tsx(3 hunks)apps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts(3 hunks)crates/audio/src/latency.rs(1 hunks)crates/camera-ffmpeg/src/macos.rs(2 hunks)crates/camera-ffmpeg/src/windows.rs(5 hunks)crates/camera-windows/src/lib.rs(3 hunks)crates/frame-converter/src/d3d11.rs(1 hunks)crates/frame-converter/src/videotoolbox.rs(5 hunks)crates/media-info/src/lib.rs(1 hunks)crates/recording/src/sources/microphone.rs(4 hunks)crates/scap-targets/src/platform/win.rs(14 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
crates/audio/src/latency.rsapps/desktop/src-tauri/src/audio_meter.rsapps/desktop/src-tauri/src/camera.rscrates/media-info/src/lib.rsapps/desktop/src/routes/screenshot-editor/useScreenshotExport.tscrates/camera-windows/src/lib.rscrates/scap-targets/src/platform/win.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/windows.rscrates/camera-ffmpeg/src/windows.rscrates/recording/src/sources/microphone.rscrates/frame-converter/src/videotoolbox.rsapps/desktop/src/routes/in-progress-recording.tsxcrates/camera-ffmpeg/src/macos.rscrates/frame-converter/src/d3d11.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
crates/audio/src/latency.rsapps/desktop/src-tauri/src/audio_meter.rsapps/desktop/src-tauri/src/camera.rscrates/media-info/src/lib.rscrates/camera-windows/src/lib.rscrates/scap-targets/src/platform/win.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/windows.rscrates/camera-ffmpeg/src/windows.rscrates/recording/src/sources/microphone.rscrates/frame-converter/src/videotoolbox.rscrates/camera-ffmpeg/src/macos.rscrates/frame-converter/src/d3d11.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
crates/audio/src/latency.rsapps/desktop/src-tauri/src/audio_meter.rsapps/desktop/src-tauri/src/camera.rscrates/media-info/src/lib.rsapps/desktop/src/routes/screenshot-editor/useScreenshotExport.tscrates/camera-windows/src/lib.rscrates/scap-targets/src/platform/win.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/windows.rscrates/camera-ffmpeg/src/windows.rscrates/recording/src/sources/microphone.rscrates/frame-converter/src/videotoolbox.rsapps/desktop/src/routes/in-progress-recording.tsxcrates/camera-ffmpeg/src/macos.rscrates/frame-converter/src/d3d11.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/audio_meter.rsapps/desktop/src-tauri/src/camera.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/windows.rs
apps/desktop/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated files: tauri.ts, queries.ts, and files under apps/desktop/src-tauri/gen/
Files:
apps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
apps/desktop/src/routes/screenshot-editor/useScreenshotExport.tsapps/desktop/src/routes/in-progress-recording.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/in-progress-recording.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/camera-ffmpeg/src/macos.rs
🧬 Code graph analysis (4)
apps/desktop/src-tauri/src/camera.rs (1)
apps/desktop/src/utils/tauri.ts (1)
CameraPreviewShape(363-363)
crates/media-info/src/lib.rs (1)
apps/desktop/src/global.d.ts (1)
Sample(79-104)
apps/desktop/src-tauri/src/recording.rs (1)
apps/desktop/src/utils/tauri.ts (2)
CameraShape(365-365)CameraPreviewShape(363-363)
crates/camera-ffmpeg/src/windows.rs (1)
crates/camera-windows/src/lib.rs (2)
new(453-473)new(499-521)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (32)
crates/audio/src/latency.rs (1)
440-448: Clamp refactor preserves previous fallback latency boundsUsing
.clamp(FALLBACK_WIRED_LATENCY_SECS, MAX_LATENCY_SECS)here is equivalent to the prior max-then-min sequence and keeps the non-macOS fallback safely within[0.03, 3.0]while making the intent clearer. Existing tests arounddefault_output_latency_hintshould continue to pass unchanged.Please confirm your MSRV supports
f64::clampand run the existing latency tests to validate behavior on your target platforms.apps/desktop/src/routes/in-progress-recording.tsx (1)
743-755: ActionButton transitions and disabled hover behavior are cleaner and more consistentSwitching to
transition-colors duration-100and adding explicit hover/active and disabled-hover styles makes these controls feel more responsive and avoids misleading hover feedback when disabled. This looks consistent with the stop button and the rest of the toolbar.apps/desktop/src-tauri/src/recording.rs (2)
6-9: LGTM!The new imports support the camera shape mapping logic added to
project_config_from_recording().Also applies to: 45-45
1684-1700: LGTM!The camera shape mapping logic correctly reads the preview state and applies appropriate configurations. The error handling gracefully continues if the preview state can't be accessed.
crates/recording/src/sources/microphone.rs (5)
125-133: LGTM!The sample size mappings are correct for all supported formats, and the function properly handles unknown formats by returning None.
153-153: LGTM!I8 format support is correctly implemented with proper casting from unsigned byte to signed i8.
160-174: LGTM!U16 and U32 format conversions are correctly implemented. Both formats can be represented exactly in f64 (f64 has 53-bit mantissa, sufficient for u32's 32 bits).
201-204: LGTM!I8 format writing is correctly implemented with proper clamping to i8 range and correct bit-pattern reinterpretation for the byte write.
213-232: LGTM!U16, U32, and U64 format writing implementations are correct, with proper clamping, rounding, and byte-order handling via to_ne_bytes.
crates/camera-ffmpeg/src/macos.rs (3)
53-89: LGTM - format code consolidation.Merging "420v" and "420f" to share the NV12 conversion path is appropriate, and the existing two-plane copy logic correctly handles both Y (plane 0) and interleaved UV (plane 1) data.
113-135: LGTM - BGRA format support.The BGRA format handler correctly allocates a 4-byte-per-pixel frame and performs row-wise copy with proper stride handling.
136-158: LGTM - ARGB format support.The ARGB format handler correctly allocates a 4-byte-per-pixel frame and performs row-wise copy with proper stride handling.
apps/desktop/src-tauri/src/audio_meter.rs (1)
143-143: Safer fallback for unknown sample formats.Returning
0.0instead of panicking withtodo!()is a reasonable improvement. This prevents crashes ifcpaladds new sample formats in future versions. Consider whether atracing::warn!on first occurrence would help diagnose cases where audio metering stops working due to an unsupported format.apps/desktop/src/routes/screenshot-editor/useScreenshotExport.ts (2)
11-12: Clean context access refactor.Storing the context in
editorCtxand destructuring commonly used properties is a clean pattern that improves readability while allowing direct property access (e.g.,editorCtx.path,editorCtx.prettyName) where needed.
286-286: Good UX improvement for export filenames.Using
prettyNamefor the default export filename provides a more meaningful name than a hardcoded "screenshot.png".crates/camera-windows/src/lib.rs (2)
463-464: MJPEG and YV12 mappings for MediaFoundation look correct.The mappings align with the standard Windows Media Foundation video format GUIDs.
511-512: MJPEG and YV12 mappings for DirectShow look correct.The DirectShow subtype mappings are consistent with the MediaFoundation mappings above.
crates/camera-ffmpeg/src/windows.rs (3)
117-131: Correct pixel format mapping for Windows ARGB.Windows stores ARGB in BGRA memory order; using
Pixel::BGRAis the correct mapping.
132-146: RGB24 path correctly maps to BGR24 with vertical flip.Windows DIBs store RGB as BGR and are bottom-up oriented. The vertical flip
(height - y - 1)andPixel::BGR24format are correct.
193-220: YV12 to YUV420P conversion correctly swaps U/V planes.YV12 stores planes as Y, V, U while YUV420P expects Y, U, V. The code correctly reads from
v_offsetintodest_vandu_offsetintodest_u, performing the necessary plane swap.apps/desktop/src-tauri/src/windows.rs (2)
361-361: Shadow settings for overlay windows are consistent and appropriate.The
TargetSelectOverlayandInProgressRecordingwindows haveshadow(false)configured unconditionally as part of their builder chain, which is consistent with their transparent overlay design. Both windows usetransparent(true)along with overlay-specific settings (skip_taskbar,always_on_top,visible_on_all_workspaces), making the absence of shadows the correct visual choice. This pattern matches the existingRecordingsOverlayimplementation, indicating a consistent design for overlay windows across the codebase.Likely an incorrect or invalid review comment.
730-731: Address lifecycle management for spawned fake window listener.The
spawn_fake_window_listenercreates an infinite polling task viatokio::spawnthat only exits when the window's entry is removed from theFakeWindowBoundsstate map. The spawned task has no automatic cleanup tied to window close events—cleanup depends entirely on the frontend callingremove_fake_window(command "n"). Verify that:
- The frontend reliably calls
remove_fake_windowwhen theInProgressRecordingwindow closes (no resource leak if cleanup is missed)- The fire-and-forget spawn pattern is appropriate; consider capturing the
JoinHandleif explicit cancellation becomes necessary in the futureThe error handling pattern using
.ok()is intentional and appropriate for best-effort cursor event management.crates/frame-converter/src/videotoolbox.rs (2)
18-22: BGRA/ARGB CoreVideo constants and mapping look correctThe new
K_CV_PIXEL_FORMAT_TYPE_32_BGRAandK_CV_PIXEL_FORMAT_TYPE_32_ARGBvalues match CoreVideo’skCVPixelFormatType_32BGRAandkCVPixelFormatType_32ARGBdefinitions, and the addedPixel::BGRA/Pixel::ARGBmatch arms correctly wire ffmpeg’s formats to those CV pixel types. (stackoverflow.com)No functional issues from my side; this should enable VT paths for BGRA/ARGB as intended.
If you want an extra sanity check against your local SDK, you can confirm these constants in
CVPixelBuffer.hor the CoreVideo reference for your targeted macOS/iOS versions.Also applies to: 83-91
29-65: New CVPixelBuffer accessors are correctly declared and usedThe added externs for
CVPixelBufferGetBaseAddress,CVPixelBufferGetBytesPerRow, andCVPixelBufferGetHeightmatch the CoreVideo signatures (taking aCVPixelBufferRefand returning a base pointer orusize), and you only call them afterCVPixelBufferLockBaseAddress, which is exactly what the docs require. (docs.rs)Given they sit in the same CoreVideo block as the other CVPixelBuffer functions, linking behavior stays consistent.
If you upgrade SDKs frequently, it’s worth quickly re-grepping
CVPixelBuffer.hin your build toolchain to ensure the typedefs forCVPixelBufferRefand the return types (size_t/void *) haven’t changed in any future Apple SDK you target.crates/scap-targets/src/platform/win.rs (8)
83-87: Primary-monitor detection refactor looks correctCombining the
GetMonitorInfoWsuccess check with the primary-flag test into a single condition preserves behavior and short‑circuits cleanly. No issues here.
200-203: DEVMODEW initialization via struct update is fineInitializing
DEVMODEWwithdmSizeset in the literal and..Default::default()for the rest is idiomatic and equivalent to the previous pattern of default‑then‑assign.
497-506: Version-info error handling is still correctSwitching to an early return on
GetFileVersionInfoW(...).is_err()keeps the same behavior and reads more clearly. The subsequent query and UTF‑16 handling remain unchanged.
669-731: High‑res executable icon search and HICON conversion look soundThe updated
extract_executable_icons_high_res,get_icon_size, andhicon_to_png_bytes_optimizedlogic appears correct:
- You walk candidate sizes and icon indices, track the best realized size, and early‑return once you hit a sufficiently large icon.
- All extracted
HICONs are destroyed after use.- Bitmaps allocated via
GetIconInfoare freed viaDeleteObjectin both success and failure paths.- Device contexts from
GetDC/CreateCompatibleDCare released in all exit paths.I don’t see functional regressions introduced by the new let‑chains or best‑icon tracking.
Also applies to: 766-791, 793-852
1230-1236: Layered/transparent window handling: confirm intended behaviorThe updated layered‑window branch now:
- Treats windows with
WS_EX_TRANSPARENTbut notWS_EX_LAYEREDas always ignored.- For layered windows, calls
GetLayeredWindowAttributesand ignores them only when the call succeeds andalpha < 50.This is a reasonable heuristic (skip nearly fully transparent overlays), but it is a behavior change if previously all transparent/layered windows were filtered out unconditionally. Please confirm this matches the UX you want for picking the “topmost” window under the cursor.
267-278: Cursor-position helper remains correct after refactorUsing
GetCursorPos(&mut point).is_ok()and mapping intoPhysicalPositionis fine and consistent with otherResult‑returning Win32 wrappers in this file. The function cleanly returnsNoneon failure.
766-771:GetIconInfoerror handling is fine, but double-check signature expectationsBoth
get_icon_sizeandhicon_to_png_bytes_optimizednow early‑return onGetIconInfo(...).is_err(). That’s consistent with the rest of the file’s Win32 usage and avoids trying to use uninitialized bitmap handles.Given the Windows bindings sometimes expose BOOL‑returning APIs as
Result<()>and sometimes as rawBOOL, it’s worth quickly confirming thatGetIconInfois indeed one of theResult‑wrapped ones in thewindowscrate version you target.Also applies to: 793-799
975-981: Potential unresolvedsize_ofsymbol
DwmGetWindowAttributecalls usesize_of::<RECT>() as u32, but this file only importsstd::memasmemand otherwise usesmem::size_of::<...>()everywhere else.Unless
size_ofis imported into scope elsewhere in this module, these lines will fail to compile. If there isn’t an existingusebringingsize_ofinto scope, consider switching tomem::size_of::<RECT>()here for consistency with the rest of the file.Also applies to: 1013-1017
| MJPEG, | ||
| YV12, | ||
| BGR24, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for BGR24, RGB24, and related Windows media format constants
rg -n "BGR24|MEDIASUBTYPE_RGB24|MFVideoFormat_RGB24|MEDIASUBTYPE_BGR24|MFVideoFormat_BGR24" --type rustRepository: CapSoftware/Cap
Length of output: 854
🏁 Script executed:
#!/bin/bash
# Check the full context of MFPixelFormat::new and DSPixelFormat::new to see all mappings
rg -n -A 30 "impl MFPixelFormat" crates/camera-windows/src/lib.rs | head -60Repository: CapSoftware/Cap
Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Check DSPixelFormat::new as well
rg -n -A 30 "impl DSPixelFormat" crates/camera-windows/src/lib.rs | head -60Repository: CapSoftware/Cap
Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Check how BGR24 is used in camera-ffmpeg for Windows context
rg -n "BGR24" crates/camera-ffmpeg/src/Repository: CapSoftware/Cap
Length of output: 398
🏁 Script executed:
#!/bin/bash
# Find all places where PixelFormat::BGR24 is instantiated or matched
rg -n "PixelFormat::BGR24" --type rustRepository: CapSoftware/Cap
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check the enum definition and any other uses
cat -n crates/camera-windows/src/lib.rs | sed -n '220,240p'Repository: CapSoftware/Cap
Length of output: 511
🏁 Script executed:
#!/bin/bash
# Let's check how RGB24 is actually used in camera-ffmpeg to understand the conversion pattern
rg -n -B 3 -A 3 "PixelFormat::RGB24" crates/camera-ffmpeg/src/windows.rsRepository: CapSoftware/Cap
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Check if there's any conversion between RGB24 and BGR24
rg -n "RGB24|BGR24" crates/camera-ffmpeg/src/windows.rs -B 2 -A 2Repository: CapSoftware/Cap
Length of output: 543
BGR24 is unreachable code and should be removed.
The BGR24 variant is added to the PixelFormat enum but never instantiated from Windows media types—MFPixelFormat::new and DSPixelFormat::new have no mapping for it. Additionally, PixelFormat::RGB24 already converts to FFmpeg's Pixel::BGR24 (line 133 of crates/camera-ffmpeg/src/windows.rs), making the match arm at line 221 for PixelFormat::BGR24 unreachable dead code. Either remove the BGR24 enum variant or clarify if it serves a purpose.
| SampleFormat::U64 => { | ||
| let mut buf = [0u8; 8]; | ||
| buf.copy_from_slice(bytes); | ||
| Some(u64::from_ne_bytes(buf) as f64) | ||
| } |
There was a problem hiding this comment.
Be aware of precision loss for U64 values above 2^53.
Converting u64 to f64 can lose precision since f64's mantissa is only 53 bits. For u64 values exceeding 9,007,199,254,740,992 (2^53), some precision will be lost during conversion and averaging. However, this is unlikely to be problematic for typical audio sample values.
🤖 Prompt for AI Agents
crates/recording/src/sources/microphone.rs around lines 180-184: the current
code converts a u64 sample directly to f64 which can lose precision for values >
2^53; change the handling so we preserve exactness until averaging — either (A)
stop converting each sample to f64: return/store the raw u64 and perform
accumulation/averaging using an integer accumulator (u128) and convert the final
averaged value to f64, or (B) if you must return f64 per-sample, clamp values
above 2^53 to the nearest representable integer and add a brief comment about
potential precision loss; implement option A when possible (modify downstream
types to accept integer samples or the accumulator) and add a one-line comment
documenting the 2^53 precision limit.
| handleMouseLeave, | ||
| ); | ||
| setDragState(null); | ||
| setDropTargetIndex(null); |
There was a problem hiding this comment.
Bug: Cleanup resets drag state breaking layer reordering
The onCleanup callback in the createEffect sets setDragState(null) and setDropTargetIndex(null). In SolidJS, this cleanup runs whenever the effect re-runs due to signal changes. Since handleDragMove updates dragState on every mouse move, the effect re-runs, cleanup executes first and nullifies the state, then the effect body reads the now-null state and skips adding listeners. This causes the drag-to-reorder functionality to break immediately after starting a drag. The state reset belongs only in finalizeDrag() which already handles it correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/camera-directshow/src/lib.rs (1)
1070-1107: Fix unsafe null dereference ofpcfetchedand use correct gate parameter inTypeEnumerator_Impl::NextAt line 1082, the condition
pcfetched.read() > 0unconditionally dereferencespcfetched, which is an optional COM out-parameter that may be NULL. Per COM specification,pcFetchedis optional and can only be safely dereferenced when guarded. Additionally, the logic checks uninitialized memory instead of the actual request count.Replace the condition with
cmediatypes > 0to gate whether items should be returned based on how many the caller requested, not the uninitialized output buffer:- if *self.index.borrow() == 0 && pcfetched.read() > 0 { + if *self.index.borrow() == 0 && cmediatypes > 0 {The later null-guard at line 1105 is already correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/camera-directshow/src/lib.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
crates/camera-directshow/src/lib.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
crates/camera-directshow/src/lib.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
crates/camera-directshow/src/lib.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
crates/camera-directshow/src/lib.rs (1)
4-12: Import additions forManuallyDropandDurationlook correctBoth
ManuallyDropandDurationare used in this file, and the import block remains clean and minimal. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/camera-ffmpeg/src/windows.rs(5 hunks)crates/cursor-capture/src/position.rs(1 hunks)crates/enc-ffmpeg/src/video/h264.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/cursor-capture/src/position.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
crates/enc-ffmpeg/src/video/h264.rscrates/camera-ffmpeg/src/windows.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
crates/enc-ffmpeg/src/video/h264.rscrates/camera-ffmpeg/src/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
crates/enc-ffmpeg/src/video/h264.rscrates/camera-ffmpeg/src/windows.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.896Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/enc-ffmpeg/src/video/h264.rs
🧬 Code graph analysis (1)
crates/camera-ffmpeg/src/windows.rs (1)
crates/camera-windows/src/lib.rs (3)
bytes(188-199)new(453-473)new(499-521)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/enc-ffmpeg/src/video/h264.rs (1)
169-176: LGTM: Improved readability for even dimension check.The refactoring from modulo to
is_multiple_of(2)makes the intent clearer while maintaining identical behavior. The validation correctly enforces that 4:2:0 formats require even dimensions due to chroma subsampling.crates/camera-ffmpeg/src/windows.rs (3)
193-220: LGTM! YV12 to YUV420P conversion correctly handles plane reordering.The implementation correctly converts YV12 format (Y, V, U plane order) to YUV420P (Y, U, V plane order) by reading from the V and U planes at the correct offsets and writing them to the swapped output planes.
221-234: LGTM! BGR24 path now correctly applies vertical flip.The BGR24 implementation at line 228 now correctly applies the vertical flip using
(height - y - 1) * src_stride, addressing the concern from the previous review. This is consistent with other RGB format paths that handle Windows DIB bottom-up orientation.
118-118: The pixel format mappings are correct. Windows Media Foundation format names are misleading—despite "RGB" naming, MFVideoFormat_RGB24, MFVideoFormat_RGB32, and MFVideoFormat_ARGB32 actually provide BGR, BGRA, and BGRA byte order respectively in memory. The straight memory copy to Pixel::BGR24 and Pixel::BGRA is appropriate without channel reordering.Likely an incorrect or invalid review comment.
| fn decode_mjpeg(bytes: &[u8]) -> Result<FFVideo, AsFFmpegError> { | ||
| let codec = ffmpeg::codec::decoder::find(ffmpeg::codec::Id::MJPEG) | ||
| .ok_or_else(|| AsFFmpegError::MjpegDecodeError("MJPEG codec not found".to_string()))?; | ||
|
|
||
| let decoder_context = ffmpeg::codec::context::Context::new_with_codec(codec); | ||
|
|
||
| let mut decoder = decoder_context | ||
| .decoder() | ||
| .video() | ||
| .map_err(|e| AsFFmpegError::MjpegDecodeError(format!("Failed to create decoder: {e}")))?; | ||
|
|
||
| let packet = Packet::copy(bytes); | ||
| decoder | ||
| .send_packet(&packet) | ||
| .map_err(|e| AsFFmpegError::MjpegDecodeError(format!("Failed to send packet: {e}")))?; | ||
|
|
||
| let mut decoded_frame = FFVideo::empty(); | ||
| decoder | ||
| .receive_frame(&mut decoded_frame) | ||
| .map_err(|e| AsFFmpegError::MjpegDecodeError(format!("Failed to receive frame: {e}")))?; | ||
|
|
||
| Ok(decoded_frame) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Packet::copy usage in the repository
rg "Packet::copy" --type rs -B 3 -A 3Repository: CapSoftware/Cap
Length of output: 85
🏁 Script executed:
# Search for FFVideo usage and frame validation patterns
rg "FFVideo" --type rs -B 2 -A 2Repository: CapSoftware/Cap
Length of output: 85
🏁 Script executed:
# Search for receive_frame usage patterns
rg "receive_frame" --type rs -B 3 -A 5Repository: CapSoftware/Cap
Length of output: 85
🏁 Script executed:
# Check the FFVideo struct definition
rg "struct FFVideo" --type rs -A 20Repository: CapSoftware/Cap
Length of output: 85
🏁 Script executed:
# Look for frame dimension validation patterns in codebase
rg "width\(\)|height\(\)" --type rs -B 2 -A 2 | head -50Repository: CapSoftware/Cap
Length of output: 85
🏁 Script executed:
# Search for Packet::copy usage without type filter
rg "Packet::copy" -B 3 -A 3Repository: CapSoftware/Cap
Length of output: 1215
🏁 Script executed:
# Read the windows.rs file to understand the full context
wc -l crates/camera-ffmpeg/src/windows.rsRepository: CapSoftware/Cap
Length of output: 98
🏁 Script executed:
# Read the windows.rs file
cat -n crates/camera-ffmpeg/src/windows.rs | head -80Repository: CapSoftware/Cap
Length of output: 3334
🏁 Script executed:
# Search for FFVideo struct and impl
rg "struct FFVideo|impl FFVideo" -B 2 -A 10Repository: CapSoftware/Cap
Length of output: 41
🏁 Script executed:
# Search for frame validation or dimension checks
rg "width\(\)|height\(\)|dimensions" -B 2 -A 2Repository: CapSoftware/Cap
Length of output: 50372
🏁 Script executed:
# Read more of windows.rs to see where decode_mjpeg is called and how result is used
cat -n crates/camera-ffmpeg/src/windows.rs | grep -A 20 "decode_mjpeg"Repository: CapSoftware/Cap
Length of output: 2217
🏁 Script executed:
# Check FFVideo::empty definition and whether it's validated elsewhere
rg "FFVideo::empty|receive_frame" -B 3 -A 8 | head -100Repository: CapSoftware/Cap
Length of output: 6758
🏁 Script executed:
# Look for any frame validation after decode_mjpeg result is used
rg "decode_mjpeg" -B 5 -A 15Repository: CapSoftware/Cap
Length of output: 3237
Add frame dimension validation after decoding MJPEG.
The decode_mjpeg function returns a frame without validating that decoding succeeded and produced a frame with valid dimensions. Unlike the YV12 path which explicitly sets dimensions, the decoded MJPEG frame may have zero width or height if decoding fails silently or produces incomplete data. Add a check after line 36 to validate the frame has non-zero dimensions:
let mut decoded_frame = FFVideo::empty();
decoder
.receive_frame(&mut decoded_frame)
.map_err(|e| AsFFmpegError::MjpegDecodeError(format!("Failed to receive frame: {e}")))?;
+if decoded_frame.width() == 0 || decoded_frame.height() == 0 {
+ return Err(AsFFmpegError::MjpegDecodeError("Decoded frame has zero dimensions".to_string()));
+}
+
Ok(decoded_frame)This matches the validation pattern used elsewhere in the codebase (e.g., CapturedFrame::as_ffmpeg at lines 47-49).
🤖 Prompt for AI Agents
In crates/camera-ffmpeg/src/windows.rs around lines 17 to 39, after the call to
decoder.receive_frame (around line 36) add a validation that the decoded_frame
has non-zero width and height (e.g., check decoded_frame.width() > 0 &&
decoded_frame.height() > 0); if either dimension is zero, return
Err(AsFFmpegError::MjpegDecodeError("Decoded frame has zero
dimensions".to_string())). This mirrors the CapturedFrame::as_ffmpeg pattern and
prevents returning an invalid FFVideo when MJPEG decoding yields an
empty/incomplete frame.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/camera-ffmpeg/src/windows.rs (1)
17-38: Add decoded MJPEG frame dimension validation and consider decoder reuseThe overall MJPEG decode flow (find codec → build context → send packet → receive frame) is sound, but two improvements would make this path more robust:
- Dimension validation (same concern as an earlier review)
Afterreceive_frame, you can still end up with a logically “empty” frame (e.g., width/height = 0) if decoding succeeds structurally but yields no usable image. To guard downstream code, validate the decoded dimensions before returning:let mut decoded_frame = FFVideo::empty(); decoder .receive_frame(&mut decoded_frame) .map_err(|e| AsFFmpegError::MjpegDecodeError(format!("Failed to receive frame: {e}")))?; - - Ok(decoded_frame) + if decoded_frame.width() == 0 || decoded_frame.height() == 0 { + return Err(AsFFmpegError::MjpegDecodeError( + "Decoded frame has zero dimensions".to_string(), + )); + } + + Ok(decoded_frame)This mirrors the pattern you already use to reject zero-sized frames on the capture side and avoids propagating unusable
FFVideoinstances. This repeats a concern raised in a past review on this file.If you want extra assurance, please run existing camera/FFmpeg tests that exercise MJPEG sources and confirm they still pass with this additional guard.
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/screenshot_editor.rs (1)
195-201: Consider logging errors before discarding them.Using
.ok()silently swallows potential errors fromload_for_projectandload(e.g., corrupted files, permission issues). While the fallback to dummy meta is reasonable, logging these errors would aid debugging.- let (recording_meta, loaded_config) = if let Some(cap_dir) = &cap_dir { - let meta = RecordingMeta::load_for_project(cap_dir).ok(); - let config = ProjectConfiguration::load(cap_dir).ok(); - (meta, config) + let (recording_meta, loaded_config) = if let Some(cap_dir) = &cap_dir { + let meta = RecordingMeta::load_for_project(cap_dir) + .inspect_err(|e| eprintln!("Failed to load recording meta: {e}")) + .ok(); + let config = ProjectConfiguration::load(cap_dir) + .inspect_err(|e| eprintln!("Failed to load project config: {e}")) + .ok(); + (meta, config)apps/desktop/src/routes/target-select-overlay.tsx (1)
70-70: Implementation correctly supports mode-dependent minimum crop sizes.The introduction of
MIN_SCREENSHOT_SIZE(1×1) and theminSize()function properly implements dynamic minimum size validation based on the recording mode. The validation logic inisValid()and the error messaging both correctly use this mode-dependent minimum.Optional: Consider memoizing
minSize()withcreateMemo()for consistency with other computed values in the component, though the performance impact is negligible given the simplicity of the ternary operation:-const minSize = () => - options.mode === "screenshot" ? MIN_SCREENSHOT_SIZE : MIN_SIZE; +const minSize = createMemo(() => + options.mode === "screenshot" ? MIN_SCREENSHOT_SIZE : MIN_SIZE +);Then update the usages to call it as a signal:
-const min = minSize(); +const min = minSize();(No change needed in this case since both call syntaxes work, but
createMemowould prevent unnecessary recalculations ifminSizewere used in multiple reactive contexts.)Also applies to: 435-441, 789-791
apps/desktop/src-tauri/src/tray.rs (1)
170-174: Prefer&Pathover&PathBuffor function parameters.Using
&Pathinstead of&PathBufis more idiomatic and flexible—it accepts both&Pathand&PathBufcallers without requiring conversion.fn load_single_item( - path: &PathBuf, - screenshots_dir: &PathBuf, + path: &Path, + screenshots_dir: &Path, load_thumbnail: bool, ) -> Option<CachedPreviousItem> {Add the import at the top:
use std::path::{Path, PathBuf};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src-tauri/src/screenshot_editor.rs(6 hunks)apps/desktop/src-tauri/src/tray.rs(10 hunks)apps/desktop/src/routes/target-select-overlay.tsx(4 hunks)crates/camera-ffmpeg/src/windows.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
crates/camera-ffmpeg/src/windows.rsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src-tauri/src/tray.rsapps/desktop/src-tauri/src/screenshot_editor.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
crates/camera-ffmpeg/src/windows.rsapps/desktop/src-tauri/src/tray.rsapps/desktop/src-tauri/src/screenshot_editor.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
crates/camera-ffmpeg/src/windows.rsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src-tauri/src/tray.rsapps/desktop/src-tauri/src/screenshot_editor.rs
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/target-select-overlay.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
apps/desktop/src/routes/target-select-overlay.tsx
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/tray.rsapps/desktop/src-tauri/src/screenshot_editor.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/camera-ffmpeg/src/windows.rs
🧬 Code graph analysis (4)
crates/camera-ffmpeg/src/windows.rs (1)
crates/camera-windows/src/lib.rs (1)
bytes(188-199)
apps/desktop/src/routes/target-select-overlay.tsx (1)
crates/cursor-capture/src/position.rs (1)
crop(180-182)
apps/desktop/src-tauri/src/tray.rs (2)
apps/desktop/src/utils/tauri.ts (3)
NewScreenshotAdded(435-435)NewStudioRecordingAdded(436-436)RecordingMeta(455-455)crates/project/src/meta.rs (1)
load_for_project(147-153)
apps/desktop/src-tauri/src/screenshot_editor.rs (2)
apps/desktop/src/utils/tauri.ts (1)
RecordingMeta(455-455)crates/project/src/meta.rs (1)
load_for_project(147-153)
🪛 GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
apps/desktop/src-tauri/src/tray.rs
[failure] 249-253: this if statement can be collapsed
error: this if statement can be collapsed
--> apps/desktop/src-tauri/src/tray.rs:249:17
|
249 | / if path.extension().and_then(|s| s.to_str()) == Some("cap") {
250 | | if let Some(item) = load_single_item(&path, &screenshots_dir, load_thumbnails) {
251 | | items.push(item);
252 | | }
253 | | }
| |_________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#collapsible_if
help: collapse nested if block
|
249 ~ if path.extension().and_then(|s| s.to_str()) == Some("cap")
250 ~ && let Some(item) = load_single_item(&path, &screenshots_dir, load_thumbnails) {
251 | items.push(item);
252 ~ }
|
[failure] 245-256: this if statement can be collapsed
error: this if statement can be collapsed
--> apps/desktop/src-tauri/src/tray.rs:245:5
|
245 | / if screenshots_dir.exists() {
246 | | if let Ok(entries) = std::fs::read_dir(&screenshots_dir) {
247 | | for entry in entries.flatten() {
248 | | let path = entry.path();
... |
256 | | }
| |_____^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#collapsible_if
help: collapse nested if block
|
245 ~ if screenshots_dir.exists()
246 ~ && let Ok(entries) = std::fs::read_dir(&screenshots_dir) {
247 | for entry in entries.flatten() {
...
254 | }
255 ~ }
|
[failure] 233-243: this if statement can be collapsed
error: this if statement can be collapsed
--> apps/desktop/src-tauri/src/tray.rs:233:5
|
233 | / if recordings_dir.exists() {
234 | | if let Ok(entries) = std::fs::read_dir(&recordings_dir) {
235 | | for entry in entries.flatten() {
236 | | if let Some(item) =
... |
243 | | }
| |_____^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#collapsible_if
= note: requested on the command line with -D clippy::collapsible-if
help: collapse nested if block
|
233 ~ if recordings_dir.exists()
234 ~ && let Ok(entries) = std::fs::read_dir(&recordings_dir) {
235 | for entry in entries.flatten() {
...
241 | }
242 ~ }
|
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (13)
apps/desktop/src-tauri/src/screenshot_editor.rs (6)
33-33: LGTM!The
pretty_namefield addition is consistent with the serialized type and is properly initialized fromrecording_meta.
144-168: LGTM!The image path resolution logic correctly prioritizes
original.pngand falls back to searching for any PNG. The use offlatten()gracefully handles directory entry errors.Note: If multiple PNGs exist (other than
original.png), the selected file is non-deterministic based on filesystem ordering, but this is acceptable as a fallback behavior.
183-193: LGTM!The cap directory detection logic correctly handles both scenarios: when the path itself is a
.capdirectory, and when the path is a file inside a.capdirectory.
298-298: LGTM!The
pretty_nameis correctly initialized from the loaded or fallback recording metadata.
404-411: LGTM!The
pretty_namefield addition with#[serde(rename_all = "camelCase")]correctly serializes toprettyNamefor TypeScript consumption, aligning with the updatedSerializedScreenshotEditorInstancetype intauri.ts.
436-441: LGTM!The serialized output correctly includes
pretty_namefrom the instance.apps/desktop/src/routes/target-select-overlay.tsx (1)
759-759: Transparent overlay is correct for area selection mode.The absence of backdrop classes (e.g.,
bg-black/60) on the area mode container is intentional and correct. Unlike display or window selection modes which use semi-transparent backdrops to highlight the selected target, area mode requires full transparency so users can see the content they're selecting with the cropper.apps/desktop/src-tauri/src/tray.rs (2)
556-596: Good use of background thread for thumbnail loading.The approach of initially loading items without thumbnails for fast startup, then asynchronously loading thumbnails in a background thread, is a solid pattern for maintaining UI responsiveness.
629-653: Event listeners properly handle cache updates.The listeners for
NewStudioRecordingAddedandNewScreenshotAddedcorrectly update the cache and refresh the tray menu. The screenshot listener appropriately handles both direct.cappaths and nested.pngfile paths by extracting the parent directory.crates/camera-ffmpeg/src/windows.rs (4)
3-3: New FFmpeg imports and MJPEG error variant look appropriateThe added
ffmpegimports match the new MJPEG decoding path, andAsFFmpegError::MjpegDecodeError(String)gives clear, contextual failures without affecting existing variants. No issues here.Also applies to: 7-15
51-55: Empty-byte guard before pixel-format dispatch is goodThe explicit
bytes.is_empty()check before thematchensures you never enter format-specific copy loops with an empty buffer, preventing panics from slicing intobytes. This is a simple but important safety net.
117-131: RGB/ARGB/BGR24 conversions and vertical flips look consistentThe updated branches for
ARGB,RGB24,RGB32, andBGR24now all:
- Use the appropriate FFmpeg pixel types (
Pixel::BGRA/Pixel::BGR24) to match the underlying byte layout.- Respect the per-plane stride from
FFVideorather than assuming tight packing on the destination side.- Apply the same bottom‑up to top‑down flip using
(height - y - 1)for RGB-like formats, which aligns BGR24 with the existing ARGB/RGB32 behavior.Indexing and slice sizes (
width * 4for 32‑bit,width * 3for 24‑bit) are internally consistent, and the loops don’t show obvious bounds issues given correctly sized input buffers.Also applies to: 132-146, 147-161, 221-231
192-220: YV12 → YUV420P remap and BGR24 handling look correct
- The
YV12branch correctly maps the packedY | V | Ulayout intoPixel::YUV420Pplanes, sendingUdata to plane 1 andVto plane 2 with matching(width / 2, height / 2)geometry.- The new
BGR24branch mirrors theRGB24implementation, usingPixel::BGR24, a 3‑bytesrc_stride, and the same vertical flip and stride-aware destination indexing.Assuming the capture side only reports YV12 for even dimensions (typical for 4:2:0) and provides buffers sized consistently with
width * height * 3 / 2, these branches should behave correctly without extra per-frame checks.It would be worth running a quick manual test with a YV12 camera source and a BGR24 source (if available) to visually confirm orientation and chroma ordering remain correct across devices.
Also applies to: 221-231
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/tray.rs (2)
127-133: UTF-8 slicing bug can cause panic.As previously noted by other reviewers, slicing
&title[..MAX_TITLE_LENGTH - 1]uses byte indices and will panic if the title contains multi-byte UTF-8 characters (emoji, non-ASCII text). Use character-based iteration instead.Apply this diff to fix the issue:
fn truncate_title(title: &str) -> String { if title.len() <= MAX_TITLE_LENGTH { title.to_string() } else { - format!("{}…", &title[..MAX_TITLE_LENGTH - 1]) + let truncated: String = title.chars().take(MAX_TITLE_LENGTH - 1).collect(); + format!("{truncated}…") } }
231-252: Collapse nested if statements per Clippy warnings.As flagged by static analysis, these nested if statements should be collapsed using let-chains to improve readability and satisfy workspace Clippy lints.
Apply this diff:
- if recordings_dir.exists() { - if let Ok(entries) = std::fs::read_dir(&recordings_dir) { - for entry in entries.flatten() { - if let Some(item) = load_single_item(&entry.path(), &screenshots_dir, load_thumbnails) { - items.push(item); - } + if recordings_dir.exists() + && let Ok(entries) = std::fs::read_dir(&recordings_dir) + { + for entry in entries.flatten() { + if let Some(item) = load_single_item(&entry.path(), &screenshots_dir, load_thumbnails) { + items.push(item); } } } - if screenshots_dir.exists() { - if let Ok(entries) = std::fs::read_dir(&screenshots_dir) { - for entry in entries.flatten() { - let path = entry.path(); - if path.extension().and_then(|s| s.to_str()) == Some("cap") - && let Some(item) = load_single_item(&path, &screenshots_dir, load_thumbnails) - { - items.push(item); - } + if screenshots_dir.exists() + && let Ok(entries) = std::fs::read_dir(&screenshots_dir) + { + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().and_then(|s| s.to_str()) == Some("cap") + && let Some(item) = load_single_item(&path, &screenshots_dir, load_thumbnails) + { + items.push(item); } } }As per coding guidelines, workspace Clippy lints should be applied.
🧹 Nitpick comments (4)
apps/desktop/src-tauri/src/captions.rs (1)
1013-1019: Replaceflatten()withmap_while(Result::ok)to handle persistent I/O errors.Clippy correctly warns that
flatten()will loop forever ifreader.lines()repeatedly produces I/O errors. Usingmap_while(Result::ok)stops iteration on the first error, preventing an infinite loop.Apply this diff:
- for line in reader.lines().flatten() { + for line in reader.lines().map_while(Result::ok) { if let Some(stripped) = line.strip_prefix("STDERR:") { log::info!("[WhisperX] {}", stripped); } else { log::info!("[WhisperX stderr] {}", line); } }apps/desktop/src-tauri/src/tray.rs (3)
93-94: Remove unused field or implement its usage.The
linkfield is marked with#[allow(dead_code)]and is never read. The field is populated frommeta.sharingat line 199, suggesting it may have been intended for future use. Either implement functionality to use this link (e.g., preferring the web link over local file when available) or remove the field entirely.If the field is not needed, apply this diff:
enum PreviousItemType { StudioRecording, - InstantRecording { - #[allow(dead_code)] - link: Option<String>, - }, + InstantRecording, Screenshot, }And update line 198-200:
- RecordingMetaInner::Instant(_) => PreviousItemType::InstantRecording { - link: meta.sharing.as_ref().map(|s| s.link.clone()), - }, + RecordingMetaInner::Instant(_) => PreviousItemType::InstantRecording,
431-436: Handle path conversion failure explicitly.Using
unwrap_or_default()returns an empty string if the path contains invalid UTF-8. Opening an empty path could fail silently. Consider usingto_str()with explicit error handling instead.Apply this diff:
- let mp4_path = path.join("content/output.mp4"); - if mp4_path.exists() { - let _ = app - .opener() - .open_path(mp4_path.to_str().unwrap_or_default(), None::<String>); + let mp4_path = path.join("content/output.mp4"); + if mp4_path.exists() { + if let Some(path_str) = mp4_path.to_str() { + let _ = app.opener().open_path(path_str, None::<String>); + } }
552-593: Consider adding error logging in background thumbnail thread.The background thread for loading thumbnails has no error logging. If thumbnail loading fails or encounters unexpected errors, they will be silently ignored. Adding tracing calls would help diagnose issues in production.
Consider adding logging at key points:
std::thread::spawn(move || { let screenshots_dir = screenshots_path(&app_clone); let items_needing_thumbnails: Vec<PathBuf> = { let cache_guard = cache_clone.lock().unwrap(); cache_guard .items .iter() .filter(|item| item.thumbnail.is_none()) .map(|item| item.path.clone()) .collect() }; if items_needing_thumbnails.is_empty() { return; } + tracing::debug!("Loading thumbnails for {} items", items_needing_thumbnails.len()); + for path in items_needing_thumbnails { if let Some(updated_item) = load_single_item(&path, &screenshots_dir, true) { let mut cache_guard = cache_clone.lock().unwrap(); if let Some(existing) = cache_guard.items.iter_mut().find(|i| i.path == path) { existing.thumbnail = updated_item.thumbnail; existing.thumbnail_width = updated_item.thumbnail_width; existing.thumbnail_height = updated_item.thumbnail_height; } + } else { + tracing::warn!("Failed to load thumbnail for {:?}", path); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src-tauri/src/captions.rs(2 hunks)apps/desktop/src-tauri/src/export.rs(1 hunks)apps/desktop/src-tauri/src/tray.rs(10 hunks)crates/frame-converter/src/pool.rs(1 hunks)crates/recording/src/feeds/camera.rs(6 hunks)crates/recording/src/output_pipeline/async_camera.rs(1 hunks)crates/recording/src/sources/camera.rs(2 hunks)crates/recording/src/sources/native_camera.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/recording/src/feeds/camera.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src-tauri/src/export.rscrates/recording/src/output_pipeline/async_camera.rscrates/recording/src/sources/camera.rscrates/recording/src/sources/native_camera.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/tray.rscrates/frame-converter/src/pool.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
apps/desktop/src-tauri/src/export.rscrates/recording/src/output_pipeline/async_camera.rscrates/recording/src/sources/camera.rscrates/recording/src/sources/native_camera.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/tray.rscrates/frame-converter/src/pool.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/export.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/tray.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/export.rscrates/recording/src/output_pipeline/async_camera.rscrates/recording/src/sources/camera.rscrates/recording/src/sources/native_camera.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/tray.rscrates/frame-converter/src/pool.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/output_pipeline/async_camera.rscrates/recording/src/sources/camera.rscrates/recording/src/sources/native_camera.rs
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/captions.rs (2)
crates/audio/src/audio_data.rs (1)
channels(162-164)crates/rendering/src/project_recordings.rs (1)
Some(197-204)
apps/desktop/src-tauri/src/tray.rs (3)
apps/desktop/src/utils/tauri.ts (7)
NewScreenshotAdded(435-435)NewStudioRecordingAdded(436-436)RecordingStarted(460-460)RecordingStopped(462-462)RequestOpenRecordingPicker(465-465)RequestOpenSettings(466-466)RecordingTargetMode(463-463)apps/desktop/src-tauri/src/lib.rs (9)
std(1868-1886)std(1911-1933)app(576-576)app(1649-1650)app(2722-2722)app(2758-2758)app(2835-2835)app(3060-3061)None(3199-3199)crates/project/src/meta.rs (1)
load_for_project(147-153)
🪛 GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
apps/desktop/src-tauri/src/captions.rs
[warning] 1013-1013: flatten() will run forever if the iterator repeatedly produces an Err
warning: flatten() will run forever if the iterator repeatedly produces an Err
--> apps/desktop/src-tauri/src/captions.rs:1013:36
|
1013 | for line in reader.lines().flatten() {
| ^^^^^^^^^ help: replace with: map_while(Result::ok)
|
note: this expression returning a std::io::Lines may produce an infinite number of Err in case of a read error
--> apps/desktop/src-tauri/src/captions.rs:1013:21
|
1013 | for line in reader.lines().flatten() {
| ^^^^^^^^^^^^^^
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#lines_filter_map_ok
= note: #[warn(clippy::lines_filter_map_ok)] on by default
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
apps/desktop/src-tauri/src/export.rs (1)
3-3: LGTM! Clean removal of unused import.Removing the unused
XYimport improves code clarity and reduces unnecessary dependencies.apps/desktop/src-tauri/src/captions.rs (1)
118-140: LGTM! Audio mixing logic is now clearer and correct.The refactored channel mismatch handling properly converts both the existing mixed samples and the new source to mono when needed, and correctly updates
channel_count. The flattened else-if structure is easier to follow than the previous nested approach.crates/recording/src/sources/camera.rs (1)
72-89:is_multiple_of(30)keeps logging cadence unchangedUsing
sent_count.is_multiple_of(30)anddropped_count.is_multiple_of(30)preserves the previous% 30 == 0behavior for the constant 30 and keeps the logging frequency identical; no further changes needed here.crates/recording/src/sources/native_camera.rs (1)
71-88: Native camera frame counters useis_multiple_of(30)correctlyThe switch to
sent_count.is_multiple_of(30)anddropped_count.is_multiple_of(30)retains the prior% 30 == 0logic and logging cadence, with no impact on control flow.crates/recording/src/output_pipeline/async_camera.rs (1)
252-275: Progress tracing viais_multiple_of(60)is behavior‑preservingChecking
self.frames_submitted.is_multiple_of(60)keeps the 60‑frame progress trace cadence the same as the old% 60 == 0condition, with no side effects on encoding behavior.crates/frame-converter/src/pool.rs (1)
186-193: Dropped‑frame warning condition unchanged withis_multiple_of(30)Using
dropped.is_multiple_of(30)instead ofdropped % 30 == 0keeps the warning cadence effectively the same while leaving pool behavior and stats intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/recording/src/screenshot.rs (1)
413-413: Preferok_oroverok_or_elsefor cheap error construction.Since
unsupported_error()is inexpensive (wraps a static string), useok_or(unsupported_error())instead ofok_or_else(unsupported_error)for more idiomatic Rust. As per coding guidelines, workspace Clippy lints (likeor_fun_call) recommend eager evaluation for cheap operations.Apply this pattern at lines 413, 502, 506, 513, 517, 536, and 540:
- rgb_from_rgba(&buffer, width, height, stride, ChannelOrder::Bgra).ok_or_else(unsupported_error) + rgb_from_rgba(&buffer, width, height, stride, ChannelOrder::Bgra).ok_or(unsupported_error())- let display = scap_targets::Display::from_id(&id).ok_or_else(unsupported_error)?; + let display = scap_targets::Display::from_id(&id).ok_or(unsupported_error())?;- .ok_or_else(unsupported_error)?; + .ok_or(unsupported_error())?;- let window = scap_targets::Window::from_id(&id).ok_or_else(unsupported_error)?; + let window = scap_targets::Window::from_id(&id).ok_or(unsupported_error())?;Also applies to: 502-502, 506-506, 513-513, 517-517, 536-536, 540-540
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
AGENTS.md(1 hunks)CLAUDE.md(1 hunks)apps/desktop/src-tauri/src/tray.rs(10 hunks)crates/enc-mediafoundation/src/d3d.rs(1 hunks)crates/enc-mediafoundation/src/media.rs(2 hunks)crates/enc-mediafoundation/src/video/h264.rs(6 hunks)crates/mediafoundation-ffmpeg/src/audio.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(2 hunks)crates/recording/src/screenshot.rs(4 hunks)crates/recording/src/sources/screen_capture/windows.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
crates/enc-mediafoundation/src/media.rscrates/recording/src/sources/screen_capture/windows.rscrates/mediafoundation-ffmpeg/src/audio.rscrates/recording/src/screenshot.rscrates/enc-mediafoundation/src/video/h264.rscrates/recording/src/output_pipeline/win.rscrates/enc-mediafoundation/src/d3d.rsapps/desktop/src-tauri/src/tray.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
crates/enc-mediafoundation/src/media.rscrates/recording/src/sources/screen_capture/windows.rscrates/mediafoundation-ffmpeg/src/audio.rscrates/recording/src/screenshot.rscrates/enc-mediafoundation/src/video/h264.rscrates/recording/src/output_pipeline/win.rscrates/enc-mediafoundation/src/d3d.rsapps/desktop/src-tauri/src/tray.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
crates/enc-mediafoundation/src/media.rscrates/recording/src/sources/screen_capture/windows.rscrates/mediafoundation-ffmpeg/src/audio.rscrates/recording/src/screenshot.rscrates/enc-mediafoundation/src/video/h264.rscrates/recording/src/output_pipeline/win.rscrates/enc-mediafoundation/src/d3d.rsapps/desktop/src-tauri/src/tray.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/tray.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: PRs must include clear description, linked issues, screenshots/GIFs for UI changes, env/migration notes, and keep scope tight
📚 Learning: 2025-11-29T04:31:23.924Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: Applies to **/*.rs : Format all Rust code using `rustfmt` and apply workspace clippy lints
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:23.924Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: Run quality checks with `pnpm lint`, `pnpm format`, `pnpm typecheck` for TypeScript/JavaScript and `cargo build`/`cargo test` for Rust
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:05.302Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.302Z
Learning: Applies to **/*.rs : Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:05.302Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.302Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:23.924Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: Applies to **/*.{ts,tsx,js,jsx,rs} : Never add comments to code in any language (no `//`, `/* */`, `///`, `//!`, `#`, etc.). Code must be self-explanatory through naming, types, and structure
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:05.302Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.302Z
Learning: Applies to **/*.{ts,tsx,js,jsx,rs,py,sh} : Never add any form of comments to code (single-line //, multi-line /* */, documentation ///, //!, /** */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:05.302Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.302Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:23.924Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: Applies to **/*.test.{ts,tsx} : Use Vitest for TypeScript/JavaScript unit tests
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:23.924Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: Turborepo monorepo structure: `apps/desktop` (Tauri v2 + SolidStart), `apps/web` (Next.js), `apps/cli` (Rust CLI), `packages/*` for shared libs, `crates/*` for Rust crates
Applied to files:
AGENTS.md
📚 Learning: 2025-11-29T04:31:23.924Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T04:31:23.924Z
Learning: Applies to **/*.rs : Use snake_case for Rust module names and kebab-case for Rust crate names
Applied to files:
AGENTS.md
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/enc-mediafoundation/src/video/h264.rscrates/recording/src/output_pipeline/win.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/enc-mediafoundation/src/video/h264.rs
🧬 Code graph analysis (2)
crates/enc-mediafoundation/src/d3d.rs (1)
crates/recording/src/capture_pipeline.rs (1)
create_d3d_device_with_type(292-315)
apps/desktop/src-tauri/src/tray.rs (1)
apps/desktop/src/utils/tauri.ts (9)
NewScreenshotAdded(435-435)NewStudioRecordingAdded(436-436)RecordingStarted(460-460)RecordingStopped(462-462)RequestOpenRecordingPicker(465-465)RequestOpenSettings(466-466)RecordingTargetMode(463-463)ShowCapWindow(479-479)RecordingMeta(455-455)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (rust)
🔇 Additional comments (18)
AGENTS.md (1)
25-44: Documentation accurately reflects workspace lint configuration — no changes needed.The Rust Clippy Rules section correctly documents all workspace lints defined in Cargo.toml. Verification confirms:
- All 12 Clippy lints match the
[workspace.lints.clippy]configuration (each set to "deny")- The
unused_must_use = "deny"compiler lint matches[workspace.lints.rust]- Lint descriptions are accurate and well-integrated into the existing documentation structure
The documentation properly clarifies the reference to workspace clippy lints and provides clear, actionable guidance for developers.
crates/recording/src/sources/screen_capture/windows.rs (3)
11-11: LGTM: Unused import removed.The removal of
SinkExtis appropriate as it's not used anywhere in the file.
112-112: LGTM: Idiomatic Option construction.The use of
then_someis more concise and idiomatic than the previous closure-based approach.
206-206: LGTM: Non-blocking error propagation in callback.Using
try_sendwithdropis appropriate here since the close callback cannot block. The send may fail if the channel is full or the receiver is gone (during shutdown), both of which are acceptable failure modes in this context.crates/mediafoundation-ffmpeg/src/audio.rs (1)
25-25: LGTM - redundant cast removed.Since
lengthis alreadyusize, the cast was unnecessary.crates/recording/src/output_pipeline/win.rs (1)
361-366: LGTM! Clean refactoring using let-chains.The refactoring from nested if-let blocks to chained let-conditions improves readability while preserving the exact same behavior. Both
AudioMuxerimplementations benefit from this modern Rust 2024 idiom.Also applies to: 684-689
crates/enc-mediafoundation/src/media.rs (1)
25-25: LGTM: Appropriate lint suppressions.The
clippy::missing_safety_docallowances correctly align with the project's no-comments policy. These thin FFI wrappers have self-evident safety contracts.Also applies to: 35-35
crates/enc-mediafoundation/src/d3d.rs (1)
49-53: LGTM: Idiomatic let-chain pattern.The refactor flattens nested conditionals using let-chains, improving readability while preserving the WARP fallback behavior.
crates/enc-mediafoundation/src/video/h264.rs (5)
14-14: LGTM: Cleanup of unused import.Removing
E_FAILis correct; onlyE_NOTIMPLis referenced in this file.
104-104: LGTM: Reasonable lint suppression.The allowance is appropriate for this internal configuration helper with distinct, necessary parameters.
214-217: LGTM: Explicit transmute improves clarity.The explicit type annotation
transmute::<_, usize>makes the unsafe FFI cast more transparent.
246-250: LGTM: Idiomatic error-handling patterns.The let-chain refactors flatten nested conditionals in the input type negotiation loop, improving readability while preserving behavior.
Also applies to: 268-273
411-429: LGTM: Cleaner control flow.The let-chain pattern flattens the nested conditions, making it clear that frame processing requires both
!should_stopand a successful frame acquisition.apps/desktop/src-tauri/src/tray.rs (5)
141-172: LGTM!The thumbnail loading implementation correctly handles image scaling and center-cropping to produce consistent 32×32 thumbnails. The use of
FilterType::Triangleprovides good quality, and the manual center-crop logic is appropriate for the small output size.
174-263: LGTM!The item loading logic correctly handles both recordings and screenshots, with appropriate type distinction and conditional thumbnail loading. The let-chain pattern for collapsible ifs follows Rust best practices, and the SystemTime fallback has been correctly updated to use
now()instead ofUNIX_EPOCH.
265-363: LGTM!The menu creation logic properly handles both empty and populated states, uses visual indicators for different item types, and correctly integrates thumbnails as menu icons. The structure is clear and follows Tauri's menu API patterns.
365-446: LGTM!The cache management correctly handles deduplication and ordering, and the click handler appropriately dispatches to different actions based on item type. The use of
tokio::spawnfor async operations and proper thread safety withMutexare well-implemented.
640-655: Simplify the NewScreenshotAdded event handler path logic—the else branch is unreachable.The
image_path_for_emitpassed to the event is always a PNG file path ({id}.cap/original.png), so the condition checkingextension() == Some("png")will always be true. The else branch that uses the path directly will never execute. Remove the conditional and simplify to directly take the parent directory.
| fn recordings_path(app: &AppHandle) -> PathBuf { | ||
| let path = app.path().app_data_dir().unwrap().join("recordings"); | ||
| std::fs::create_dir_all(&path).unwrap_or_default(); | ||
| path | ||
| } | ||
|
|
||
| fn screenshots_path(app: &AppHandle) -> PathBuf { | ||
| let path = app.path().app_data_dir().unwrap().join("screenshots"); | ||
| std::fs::create_dir_all(&path).unwrap_or_default(); | ||
| path | ||
| } |
There was a problem hiding this comment.
Consider logging directory creation failures.
Both recordings_path and screenshots_path use unwrap_or_default() to silently ignore errors when creating directories. While this prevents panics, it hides potential issues like permission errors or disk full conditions that could affect functionality.
Consider logging errors:
fn recordings_path(app: &AppHandle) -> PathBuf {
let path = app.path().app_data_dir().unwrap().join("recordings");
- std::fs::create_dir_all(&path).unwrap_or_default();
+ if let Err(e) = std::fs::create_dir_all(&path) {
+ tracing::warn!("Failed to create recordings directory: {e}");
+ }
path
}
fn screenshots_path(app: &AppHandle) -> PathBuf {
let path = app.path().app_data_dir().unwrap().join("screenshots");
- std::fs::create_dir_all(&path).unwrap_or_default();
+ if let Err(e) = std::fs::create_dir_all(&path) {
+ tracing::warn!("Failed to create screenshots directory: {e}");
+ }
path
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/tray.rs around lines 115 to 125, the calls to
std::fs::create_dir_all(...).unwrap_or_default() silently ignore failures;
change these to handle the Result and log errors when directory creation fails
(e.g., attempt create_dir_all(&path).map_err(|e| { log::error!("Failed to create
{} directory at {:?}: {}", "recordings"/"screenshots", &path, e); e }).ok(); )
so the function still returns the PathBuf but any permission/disk errors are
recorded for debugging.
| x: cropDialog().size.x, | ||
| y: cropDialog().size.y, | ||
| x: originalSize.x, | ||
| y: originalSize.y, |
There was a problem hiding this comment.
Bug: Crop dialog image loading fails for directory paths
When a screenshot is stored in a .cap directory, the editorInstance.path stores the directory path rather than the actual PNG file path. The crop dialog image and the fallback image loading in context both use convertFileSrc(path()) which expects a file path. When path() returns a directory (e.g., screenshots/something.cap), convertFileSrc won't produce a valid image URL, causing the crop dialog to display a broken image and the initial frame loading to fail.
Improves the screenshot editor experience with better UI organization, keyboard shortcuts, and rendering quality fixes.
Changes
UI/UX Improvements
prettyNamesupport for cleaner display namesRendering Fixes
Refactors
Note
Overhauls the screenshot editor (layers panel, new hotkeys, improved popovers and crop/preview), adds a dynamic “Previous” tray submenu with thumbnails, improves rendering/AA, and broadens camera/audio format handling, with supporting backend and CI updates.
prettyNamefor default filename; plumbedpretty_namethrough Tauri API.Previoussubmenu with recent recordings/screenshots (thumbnails, icons), open handlers, and live updates viaNewStudioRecordingAdded/NewScreenshotAdded.BGRA/ARGB/BGR24/y420paths; Windows addsMJPEG/YV12/BGR24decode and format fixes.SampleFormatvariants; safer size/convert helpers.CameraPreviewStatefields; map shapes toCameraShapein project config..capdirs; includepretty_namein instance; robust stderr parsing; replacetodo!()with fallback values.clippymatrix.AGENTS.md/CLAUDE.md; recommend Biome in VSCode.Expand-Archive; minor codesign path fix.Written by Cursor Bugbot for commit 6edcee7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.