Skip to content

Conversation

@richiemcilroy
Copy link
Member

@richiemcilroy richiemcilroy commented Aug 25, 2025

Introduces a new background border configuration feature in the ConfigSidebar component, with UI controls for configuring a border around the background, including toggling, width, color, and opacity.

Demo:

Border.Demo.mp4

Summary by CodeRabbit

  • New Features

    • Add configurable background border with toggle, width, color, and opacity controls, including sensible defaults.
    • Smoother wallpaper selection via debounced updates to prevent jitter and batch changes.
  • Bug Fixes

    • Corrected video preview path to ensure segment previews load reliably.
    • Color picker now accurately reflects the current RGB value.
  • Tests

    • Updated rendering tests to cover scenarios without a border to maintain backward compatibility.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 25, 2025

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch editor-border

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (11)
packages/ui-solid/src/auto-imports.d.ts (1)

9-87: Minimize unrelated churn: consider excluding/reverting formatting-only diffs in this generated file

This PR’s main goal is the editor border feature; these formatting-only changes to a generated .d.ts add review noise. If they came from a local generator change, either pin the generator versions and re-run to standardize output, or move this file’s re-gen into a separate “chore: re-generate dts” commit.

crates/project/src/configuration.rs (2)

196-204: BorderConfiguration: solid addition; consider minimal docs for units

Looks good. To reduce future ambiguity, add short doc comments on units and ranges (e.g., "width in pixels", "opacity 0–100"). This aligns with how other fields (e.g., ShadowConfiguration) are documented inline.

 pub struct BorderConfiguration {
-    pub enabled: bool,
-    pub width: f32,     // Border width in pixels
-    pub color: Color,   // Border color (RGB)
-    pub opacity: f32,   // Border opacity (0-100)
+    /// Whether the border should be rendered
+    pub enabled: bool,
+    /// Border width in pixels
+    pub width: f32,
+    /// Border color (RGB, 0–255 per channel)
+    pub color: Color,
+    /// Border opacity in percent (0–100)
+    pub opacity: f32,
 }

222-231: Provide a reusable default to avoid duplication across crates

You already implement Default here; consider also exposing a reusable constant to prevent copying literal defaults in other modules (e.g., rendering). This avoids drift if defaults ever change.

+pub const DEFAULT_BORDER: BorderConfiguration = BorderConfiguration {
+    enabled: false,
+    width: 5.0,
+    color: [255, 255, 255],
+    opacity: 80.0,
+};
+
 impl Default for BorderConfiguration {
     fn default() -> Self {
-        Self {
-            enabled: false,
-            width: 5.0,
-            color: [255, 255, 255], // White
-            opacity: 80.0,          // 80% opacity
-        }
+        DEFAULT_BORDER
     }
 }

If you export this, downstream code can reference cap_project::DEFAULT_BORDER instead of re-declaring the struct literal.

crates/rendering/src/lib.rs (1)

639-651: Minor: simplify border_enabled check

Non-blocking: you can tighten the enable check.

-                border_enabled: if project
-                    .background
-                    .border
-                    .as_ref()
-                    .map_or(false, |b| b.enabled)
-                {
+                border_enabled: if project.background.border.as_ref().is_some_and(|b| b.enabled) {
                     1.0
                 } else {
                     0.0
                 },

Note: Option::is_some_and requires Rust 1.70+. Use the existing map_or if your MSRV is lower.

crates/rendering-skia/src/layers/background.rs (1)

249-254: Minor: Avoid calling into render when width is zero.

You already early-return in render_border, but checking width here avoids a paint setup and call overhead.

-        if let Some(border) = &uniforms.border {
-            if border.enabled {
-                self.render_border(canvas, bounds, border);
-            }
-        }
+        if let Some(border) = &uniforms.border {
+            if border.enabled && border.width > 0.0 {
+                self.render_border(canvas, bounds, border);
+            }
+        }
crates/rendering/src/composite_frame.rs (2)

29-35: Uniform padding/alignment: Rust struct adds explicit padding fields not in WGSL.

The added _padding0 and _padding1b fields compensate for WGSL’s implicit padding (after two f32s to align vec2, and between vec2 and vec4). This is correct and prevents UB when mapping to the WGSL layout.

Recommendation:

  • Add a short comment explaining the rationale (std140-like rules for uniforms), so future edits don’t break alignment.
  • Consider adding a small unit test asserting size_of::<CompositeVideoFrameUniforms>() matches wgsl reflection if you have a reflection path, or at least documenting expected size.

101-133: Bind group layout still compatible; consider setting min_binding_size.

Leaving min_binding_size: None is permissible, but setting it to NonZeroU64::new(size_of::<CompositeVideoFrameUniforms>() as u64) helps validation catch layout regressions at runtime.

crates/rendering/src/shaders/composite-video-frame.wgsl (4)

17-21: Uniforms: new border fields look correct; document implicit padding.

WGSL will insert implicit padding before _padding1 (to align vec2) and between _padding1 and border_color (to align vec4). Host-side Rust explicitly represents these as _padding0 and _padding1b. Add a comment here noting that host adds extra padding fields to match layout.


97-113: Border compositing path: apply global opacity and tighten alpha at edges.

  • Consider multiplying border alpha by uniforms.opacity so global fades affect the border too (if intended by UX; matches how base_color uses opacity).
  • The current edge AA uses 1px smoothstep. That’s fine; just flagging that it is in framebuffer pixels.

Proposed adjustment:

-            let border_alpha = edge_alpha * uniforms.border_color.w;
+            let border_alpha = edge_alpha * uniforms.border_color.w * uniforms.opacity;

Would you like me to add a toggle so the border can optionally ignore content opacity?


58-58: Remove unused variable dist.

dist is computed but no longer used. Drop it to avoid warnings.

-    let dist = sdf_rounded_rect(p - center, size, uniforms.rounding_px);
+    // (unused)

95-95: Remove unused bg_color.

This local is unused and can be removed.

-    let bg_color = vec4<f32>(0.0);
+    // (removed)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 096579e and 2b775b0.

📒 Files selected for processing (9)
  • apps/desktop/src/utils/tauri.ts (2 hunks)
  • crates/project/src/configuration.rs (3 hunks)
  • crates/rendering-skia/src/layers/background.rs (5 hunks)
  • crates/rendering-skia/src/layers/mod.rs (1 hunks)
  • crates/rendering/src/composite_frame.rs (2 hunks)
  • crates/rendering/src/layers/display.rs (1 hunks)
  • crates/rendering/src/lib.rs (3 hunks)
  • crates/rendering/src/shaders/composite-video-frame.wgsl (4 hunks)
  • packages/ui-solid/src/auto-imports.d.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable

Files:

  • packages/ui-solid/src/auto-imports.d.ts
  • apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use strict TypeScript and avoid any; prefer shared types from packages

Files:

  • packages/ui-solid/src/auto-imports.d.ts
  • apps/desktop/src/utils/tauri.ts
**/tauri.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Never edit auto-generated IPC bindings file: tauri.ts

Files:

  • apps/desktop/src/utils/tauri.ts
apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app

Files:

  • apps/desktop/src/utils/tauri.ts
🧠 Learnings (3)
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules

Applied to files:

  • packages/ui-solid/src/auto-imports.d.ts
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly

Applied to files:

  • apps/desktop/src/utils/tauri.ts
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/src-tauri/**/*.rs : Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types

Applied to files:

  • apps/desktop/src/utils/tauri.ts
🧬 Code graph analysis (5)
crates/rendering-skia/src/layers/mod.rs (1)
apps/desktop/src/utils/tauri.ts (1)
  • BorderConfiguration (330-330)
crates/rendering-skia/src/layers/background.rs (3)
apps/desktop/src/utils/tauri.ts (1)
  • BorderConfiguration (330-330)
crates/project/src/configuration.rs (8)
  • default (47-51)
  • default (223-230)
  • default (234-246)
  • default (309-325)
  • default (329-335)
  • default (361-369)
  • default (414-428)
  • default (565-580)
crates/rendering/src/composite_frame.rs (1)
  • default (39-64)
crates/rendering/src/lib.rs (1)
apps/desktop/src/utils/tauri.ts (1)
  • BorderConfiguration (330-330)
crates/project/src/configuration.rs (2)
apps/desktop/src/utils/tauri.ts (1)
  • BorderConfiguration (330-330)
crates/rendering/src/composite_frame.rs (1)
  • default (39-64)
apps/desktop/src/utils/tauri.ts (5)
apps/desktop/src-tauri/src/windows.rs (1)
  • id (629-655)
crates/displays/src/lib.rs (4)
  • id (28-30)
  • id (118-120)
  • name (40-42)
  • name (150-152)
crates/displays/src/platform/macos.rs (4)
  • id (44-44)
  • id (288-290)
  • name (123-142)
  • name (320-332)
crates/displays/src/platform/win.rs (5)
  • id (129-129)
  • id (801-803)
  • name (218-255)
  • name (1304-1324)
  • name (1316-1321)
apps/desktop/src/store/captions.ts (1)
  • CaptionSettings (12-25)
⏰ 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). (1)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (13)
packages/ui-solid/src/auto-imports.d.ts (2)

7-7: export {} change is harmless; keeps the file a module

Dropping the semicolon has no functional impact in a .d.ts and still correctly marks the file as a module so declare global {} works.


9-87: No action required: auto-generated file formatting is consistent given pinned plugin versions

  • We’ve confirmed that both unplugin-auto-import and unplugin-icons are pinned at ^0.18.2 and ^0.19.2 in packages/ui-solid/package.json and apps/desktop/package.json.
  • With these caret-ranges, patch-level updates won’t alter the import-style output; the generator will continue emitting mixed quote and bracket styles until you change its configuration.
  • Because this file is fully auto-generated (and has Prettier/ESLint disabled), manual edits aren’t recommended.
  • If you require a consistent import-style (single vs. double quotes, ['default'] vs. ["default"]), adjust your icon plugin or Prettier settings, or pin to exact plugin versions rather than caret-ranges.
crates/project/src/configuration.rs (2)

218-221: Optional by default: correct for backward compatibility

Adding #[serde(default)] pub border: Option<BorderConfiguration> is the right choice to avoid breaking older project-config files. No action needed.


244-245: Default background keeps border disabled: consistent with PR intent

Keeping border: None in BackgroundConfiguration::default() maintains existing behavior for saved projects. Good call.

apps/desktop/src/utils/tauri.ts (4)

328-333: Types line up with Rust: BackgroundConfiguration.border and new BorderConfiguration

The Specta-generated TS types match the Rust surface: border is optional/null-able and color is [number, number, number]. This should interop cleanly with the renderer expecting 0–255 channel ints.


128-133: Tuple array parentheses are redundant; verify generator output

The return types for listRecordings/listScreenshots are emitted as ([string, T])[]. The parentheses are harmless but stylistically redundant vs. [string, T][]. If this wasn’t regenerated by Specta, please re-generate; if it was, consider a follow-up to the generator.

Would you like me to open a ticket to tidy this in the codegen rather than hand-editing this file (per guideline: never edit tauri.ts manually)?


458-464: Import adjustments LGTM

Switching to value import alias for Channel and type-annotated value import for WebviewWindow is valid in TS 5.x and works with strict mode. No concerns.


481-488: Event factory shape: OK; ensures both global and window-scoped listeners

The callable proxy pattern lets you do events.foo.listen(cb) or events.foo(window).listen(cb). Good ergonomics and types appear sound.

crates/rendering-skia/src/layers/mod.rs (1)

14-16: No additional action needed: needs_update already checks border changes

I’ve verified that BackgroundLayer::needs_update compares the previous border state against the new uniforms.border:

  • In crates/rendering-skia/src/layers/background.rs (lines 269–273), the code includes || self.last_rendered_border != new_border, ensuring cache invalidation when the border configuration changes.

Since border comparisons are already implemented, no further changes are required here.

crates/rendering-skia/src/layers/background.rs (3)

264-273: Good: needs_update accounts for border changes.

Comparing last_rendered_border against uniforms.border ensures border edits invalidate the cache. Nice.


354-387: The scripts above will confirm whether SkiaProjectUniforms now includes a border field. Once we have that, we can update the tests accordingly.


124-131: The scripts will confirm the exact type of BorderConfiguration.color and how its defaults are defined, so we can validate whether the >> 8 shifts or the defaults themselves are incorrect. Once we have the output, we can finalize the recommended changes.

crates/rendering/src/composite_frame.rs (1)

56-63: Defaults align with shader expectations.

border_enabled: 0.0, border_width: 5.0, and border_color: [1.0, 1.0, 1.0, 0.8] are sane defaults and match the editor defaults described. No issues.

Comment on lines 133 to 139
let inset = border.width / 2.0;
let border_rect = Rect::from_xywh(
bounds.left() + inset,
bounds.top() + inset,
bounds.width() - border.width,
bounds.height() - border.width,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Guard against negative or degenerate rect sizes for thick borders.

When border.widthbounds.width() or bounds.height(), Rect::from_xywh receives negative width/height. Clamp to zero to avoid drawing issues.

-        let border_rect = Rect::from_xywh(
-            bounds.left() + inset,
-            bounds.top() + inset,
-            bounds.width() - border.width,
-            bounds.height() - border.width,
-        );
+        let border_rect = Rect::from_xywh(
+            bounds.left() + inset,
+            bounds.top() + inset,
+            (bounds.width() - border.width).max(0.0),
+            (bounds.height() - border.width).max(0.0),
+        );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let inset = border.width / 2.0;
let border_rect = Rect::from_xywh(
bounds.left() + inset,
bounds.top() + inset,
bounds.width() - border.width,
bounds.height() - border.width,
);
let inset = border.width / 2.0;
let border_rect = Rect::from_xywh(
bounds.left() + inset,
bounds.top() + inset,
(bounds.width() - border.width).max(0.0),
(bounds.height() - border.width).max(0.0),
);
🤖 Prompt for AI Agents
In crates/rendering-skia/src/layers/background.rs around lines 133 to 139, the
computed border_rect can get negative width/height when border.width >=
bounds.width() or bounds.height(); change the math to clamp the computed width
and height to a minimum of 0.0 (e.g., compute let w = (bounds.width() -
border.width).max(0.0) and let h = (bounds.height() - border.width).max(0.0) and
pass those into Rect::from_xywh while keeping the inset-based x/y), so
Rect::from_xywh never receives negative dimensions.

Comment on lines 78 to 85
self.uniforms_buffer = uniforms.to_buffer(device);

self.bind_group = Some(self.pipeline.bind_group(
device,
&self.uniforms_buffer,
&self.frame_texture_view,
));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid per-frame uniform buffer reallocation and bind-group churn

Recreating the GPU buffer and bind group every frame is expensive and can hurt throughput. Prefer updating the existing buffer with queue.write_buffer and only rebind on resize or when the buffer is actually recreated.

Apply this diff:

-        self.uniforms_buffer = uniforms.to_buffer(device);
-
-        self.bind_group = Some(self.pipeline.bind_group(
-            device,
-            &self.uniforms_buffer,
-            &self.frame_texture_view,
-        ));
+        // Update existing uniform buffer in place; bind group remains valid.
+        uniforms.write_to_buffer(queue, &self.uniforms_buffer);

And add a helper on the uniforms type (in crates/rendering/src/composite_frame.rs) to encapsulate the write:

// in composite_frame.rs
impl CompositeVideoFrameUniforms {
    pub fn write_to_buffer(&self, queue: &wgpu::Queue, buffer: &wgpu::Buffer) {
        // Requires CompositeVideoFrameUniforms: Pod + Zeroable and #[repr(C)]
        queue.write_buffer(buffer, 0, bytemuck::bytes_of(self));
    }
}

If bytemuck traits are not yet derived on CompositeVideoFrameUniforms, derive them and ensure #[repr(C)] is set to keep WGSL layout parity.

Comment on lines 639 to 671
border_enabled: if project
.background
.border
.as_ref()
.map_or(false, |b| b.enabled)
{
1.0
} else {
0.0
},
border_width: project.background.border.as_ref().map_or(5.0, |b| b.width),
_padding0: 0.0,
_padding1: [0.0; 2],
_padding1b: [0.0; 2],
border_color: {
let border = project.background.border.as_ref().unwrap_or(
&cap_project::BorderConfiguration {
enabled: false,
width: 5.0,
color: [255, 255, 255],
opacity: 80.0,
},
);

[
border.color[0] as f32 / 255.0,
border.color[1] as f32 / 255.0,
border.color[2] as f32 / 255.0,
(border.opacity / 100.0).clamp(0.0, 1.0),
]
},
_padding2: [0.0; 4],
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Bug: Possible lifetime error by taking a reference to a temporary in unwrap_or

unwrap_or(&cap_project::BorderConfiguration { ... }) on an Option<&BorderConfiguration> tries to return a reference to a temporary struct literal. That reference cannot outlive the call and will be rejected (or worse, if coerced) by the borrow checker. Use a value-producing path instead of borrowing a temporary.

Apply this diff to compute the color without holding a reference to a temporary:

-                border_color: {
-                    let border = project.background.border.as_ref().unwrap_or(
-                        &cap_project::BorderConfiguration {
-                            enabled: false,
-                            width: 5.0,
-                            color: [255, 255, 255],
-                            opacity: 80.0,
-                        },
-                    );
-
-                    [
-                        border.color[0] as f32 / 255.0,
-                        border.color[1] as f32 / 255.0,
-                        border.color[2] as f32 / 255.0,
-                        (border.opacity / 100.0).clamp(0.0, 1.0),
-                    ]
-                },
+                border_color: if let Some(b) = project.background.border.as_ref() {
+                    [
+                        b.color[0] as f32 / 255.0,
+                        b.color[1] as f32 / 255.0,
+                        b.color[2] as f32 / 255.0,
+                        (b.opacity / 100.0).clamp(0.0, 1.0),
+                    ]
+                } else {
+                    [1.0, 1.0, 1.0, 0.8]
+                },

Optionally, if you adopt a shared default constant (see configuration.rs suggestion), you could use cap_project::DEFAULT_BORDER and avoid magic numbers entirely.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
border_enabled: if project
.background
.border
.as_ref()
.map_or(false, |b| b.enabled)
{
1.0
} else {
0.0
},
border_width: project.background.border.as_ref().map_or(5.0, |b| b.width),
_padding0: 0.0,
_padding1: [0.0; 2],
_padding1b: [0.0; 2],
border_color: {
let border = project.background.border.as_ref().unwrap_or(
&cap_project::BorderConfiguration {
enabled: false,
width: 5.0,
color: [255, 255, 255],
opacity: 80.0,
},
);
[
border.color[0] as f32 / 255.0,
border.color[1] as f32 / 255.0,
border.color[2] as f32 / 255.0,
(border.opacity / 100.0).clamp(0.0, 1.0),
]
},
_padding2: [0.0; 4],
}
border_enabled: if project
.background
.border
.as_ref()
.map_or(false, |b| b.enabled)
{
1.0
} else {
0.0
},
border_width: project.background.border.as_ref().map_or(5.0, |b| b.width),
_padding0: 0.0,
_padding1: [0.0; 2],
_padding1b: [0.0; 2],
border_color: if let Some(b) = project.background.border.as_ref() {
[
b.color[0] as f32 / 255.0,
b.color[1] as f32 / 255.0,
b.color[2] as f32 / 255.0,
(b.opacity / 100.0).clamp(0.0, 1.0),
]
} else {
[1.0, 1.0, 1.0, 0.8]
},
_padding2: [0.0; 4],
🤖 Prompt for AI Agents
In crates/rendering/src/lib.rs around lines 639 to 671, the code calls
unwrap_or(&cap_project::BorderConfiguration { ... }) on an
Option<&BorderConfiguration>, which creates a reference to a temporary and can
produce a lifetime error; fix this by computing the border_color without taking
a reference to a temporary — e.g. replace the unwrap_or usage with a
value-producing path such as using as_ref().map_or(...) or unwrap_or_else(...)
that returns the computed color directly (or clone/use a static DEFAULT_BORDER
value and then index into that owned value), so the code builds the [r,g,b,a]
array inside the map_or/closure instead of trying to borrow a transient struct.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)

2132-2136: Same path newline bug in ZoomSegmentConfig

Identical issue as above. The newline inside the template literal yields an invalid path; consolidate to one line.

Apply:

- const path = convertFileSrc(
-   // TODO: this shouldn't be so hardcoded
-   `${
-     editorInstance.path
-   }/content/segments/segment-${segmentIndex()}/display.mp4`,
- );
+ const path = convertFileSrc(
+   // TODO: this shouldn't be so hardcoded
+   `${editorInstance.path}/content/segments/segment-${segmentIndex()}/display.mp4`,
+ );
🧹 Nitpick comments (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)

1539-1555: DRY up repeated default object creation for border updates

The same fallback object for border is repeated three times. Consolidate into a small helper to reduce duplication and prevent future drift if defaults change.

Apply this diff within the selected ranges:

- onChange={(v) =>
-   setProject("background", "border", {
-     ...(project.background.border ?? {
-       enabled: true,
-       width: 5.0,
-       color: [255, 255, 255],
-       opacity: 80.0,
-     }),
-     width: v[0],
-   })
- }
+ onChange={(v) =>
+   setProject("background", "border", {
+     ...ensureBorderDefaults(),
+     width: v[0],
+   })
+ }
- onChange={(color) =>
-   setProject("background", "border", {
-     ...(project.background.border ?? {
-       enabled: true,
-       width: 5.0,
-       color: [255, 255, 255],
-       opacity: 80.0,
-     }),
-     color,
-   })
- }
+ onChange={(color) =>
+   setProject("background", "border", {
+     ...ensureBorderDefaults(),
+     color,
+   })
+ }
- onChange={(v) =>
-   setProject("background", "border", {
-     ...(project.background.border ?? {
-       enabled: true,
-       width: 5.0,
-       color: [255, 255, 255],
-       opacity: 80.0,
-     }),
-     opacity: v[0],
-   })
- }
+ onChange={(v) =>
+   setProject("background", "border", {
+     ...ensureBorderDefaults(),
+     opacity: v[0],
+   })
+ }

Add this helper inside BackgroundConfig (near other locals):

function ensureBorderDefaults() {
  return (
    project.background.border ?? {
      enabled: true,
      width: 5.0,
      color: [255, 255, 255] as [number, number, number],
      opacity: 80.0,
    }
  );
}

Also applies to: 1558-1572, 1574-1592

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2b775b0 and 09751fd.

📒 Files selected for processing (3)
  • apps/desktop/src/routes/editor/ConfigSidebar.tsx (4 hunks)
  • apps/desktop/src/utils/tauri.ts (3 hunks)
  • crates/project/src/configuration.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/project/src/configuration.rs
  • apps/desktop/src/utils/tauri.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app

Files:

  • apps/desktop/src/routes/editor/ConfigSidebar.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable

Files:

  • apps/desktop/src/routes/editor/ConfigSidebar.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use strict TypeScript and avoid any; prefer shared types from packages

Files:

  • apps/desktop/src/routes/editor/ConfigSidebar.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
apps/desktop/src/routes/editor/ui.tsx (2)
  • Field (25-47)
  • Slider (65-147)
apps/desktop/src/components/Toggle.tsx (1)
  • Toggle (37-50)
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
  • GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (4)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (4)

1513-1534: Border toggle preserves existing settings — LGTM

Good UX: enabling the toggle retains prior width/color/opacity and initializes sensible defaults when missing.


2498-2498: Binding the native color input to current value — LGTM

Setting value={rgbToHex(props.value)} keeps the picker in sync with the displayed swatch and text field. Good improvement.


1574-1592: Opacity normalization verified and handled correctly

I’ve confirmed that the UI’s 0–100 opacity values are properly converted to the renderer’s 0–1 range in both pipelines:

  • In the WebGPU path (crates/rendering/src/lib.rs), the uniform’s alpha is set via
    border_color: [ border.color[0] as f32 / 255.0, …, (border.opacity / 100.0).clamp(0.0, 1.0), ] (lines 664–667).
  • In the Skia path (crates/rendering-skia/src/layers/background.rs), the alpha is computed with
    let alpha = ((border.opacity / 100.0).clamp(0.0, 1.0) * 255.0) as u8; (line 124).

No further changes are needed for opacity conversion.


2079-2079: Verify snake_case setting key and add fallback for custom cursor capture

The custom_cursor_capture2 flag is defined in apps/desktop/src/utils/tauri.ts but your UI defaults still use the old customCursorCapture2 key in both

  • apps/desktop/src/routes/(window-chrome)/settings/general.tsx (line 120)
  • apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (line 28)

Meanwhile, in ConfigSidebar.tsx (line 2079) you only check the new snake_case key:

- disabled={!generalSettings.data?.custom_cursor_capture2}
+ disabled={!(
+   generalSettings.data?.custom_cursor_capture2 ??
+   // fallback to old key for existing users
+   (generalSettings.data as any)?.customCursorCapture2
+ )}

Without this fallback, the “Auto” option will render disabled by default for users whose stored settings still use the camelCase key. Either migrate the old key at load time or include the temporary fallback above to ensure a smooth rollout.

Comment on lines 1924 to 1927
`${
editorInstance.path
}/content/segments/segment-${segmentIndex()}/display.mp4`,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: newline inserted into file path breaks video preview

The multiline template literal inserts a newline between editorInstance.path and the rest of the path, producing an invalid URL. Use a single-line template string.

Apply:

- const path = convertFileSrc(
-   `${
-     editorInstance.path
-   }/content/segments/segment-${segmentIndex()}/display.mp4`,
- );
+ const path = convertFileSrc(
+   `${editorInstance.path}/content/segments/segment-${segmentIndex()}/display.mp4`,
+ );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
`${
editorInstance.path
}/content/segments/segment-${segmentIndex()}/display.mp4`,
);
const path = convertFileSrc(
`${editorInstance.path}/content/segments/segment-${segmentIndex()}/display.mp4`,
);
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/ConfigSidebar.tsx around lines 1924 to 1927,
the multiline template literal introduces an unintended newline into the
generated file path (breaking the video preview); replace the multiline template
string with a single-line template literal so the resulting path is contiguous
(e.g.
`${editorInstance.path}/content/segments/segment-${segmentIndex()}/display.mp4`)
ensuring no embedded whitespace or line breaks.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/rendering-skia/src/layers/background.rs (1)

356-387: Tests won’t compile after SkiaProjectUniforms shape change.

Add the new border field (None) to both initializers in tests.

         let uniforms = SkiaProjectUniforms {
             output_size: (800, 600),
             background: BackgroundSource::Color {
                 value: [65535, 0, 0],
             },
+            border: None,
         };

         // ...

         let new_uniforms = SkiaProjectUniforms {
             output_size: (800, 600),
             background: BackgroundSource::Color {
                 value: [0, 65535, 0],
             },
+            border: None,
         };
♻️ Duplicate comments (2)
crates/rendering/src/layers/display.rs (1)

78-80: In-place uniform buffer updates — nice follow-through.

This aligns with prior guidance to avoid per-frame buffer/bind-group churn.

apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)

1922-1923: Resolved: single-line template fixes broken video preview path

The newline issue is gone; path construction is now correct.

🧹 Nitpick comments (12)
crates/rendering-skia/src/bin/test_background.rs (1)

73-74: Explicitly setting border: None is correct and future-proof.

These initializations keep tests compiling against the new SkiaProjectUniforms shape. Consider adding one case with an enabled border to sanity-check rendering and cache invalidation paths.

Example change for the gradient test:

-        border: None,
+        border: Some(cap_project::BorderConfiguration {
+            enabled: true,
+            width: 6.0,
+            color: [65535, 65535, 65535],
+            opacity: 80.0, // percent
+        }),

Also applies to: 137-138, 202-203, 267-268, 313-314

apps/desktop/src/utils/tauri.ts (1)

334-337: Confirm this change was generated, not hand-edited.

Per guidelines, never hand-edit tauri.ts. Please re-run the generator if needed and commit the produced file. Also verify TS and Rust BorderConfiguration semantics (opacity range, color scale) are identical.

crates/rendering-skia/src/layers/background.rs (2)

109-142: Border rendering works; improve robustness and visuals.

  • Set stroke join to Round to avoid sharp corner artifacts.
  • Opacity appears percent-based; confirm contract to avoid double-scaling if upstream switches to 0..1.
         let mut paint = Paint::default();
         paint.set_style(skia_safe::PaintStyle::Stroke);
         paint.set_stroke_width(border.width);
         paint.set_anti_alias(true);
+        paint.set_stroke_join(skia_safe::PaintJoin::Round);

249-255: Decouple cache keys from prepare() ordering.

record() renders from uniforms.border but stores last_rendered_border from current_border (set in prepare). If record() were called without a prior prepare, cache invalidation could be off. Store the rendered value directly.

-        // Render border if enabled
-        if let Some(border) = &uniforms.border {
-            if border.enabled {
-                self.render_border(canvas, bounds, border);
-            }
-        }
+        // Render border if enabled
+        let rendered_border = uniforms.border.clone();
+        if let Some(border) = &rendered_border {
+            if border.enabled {
+                self.render_border(canvas, bounds, border);
+            }
+        }

         // Update what was last rendered
         self.last_rendered_background = self.current_background.clone();
-        self.last_rendered_border = self.current_border.clone();
+        self.last_rendered_border = rendered_border;

Also applies to: 258-259

crates/rendering/src/lib.rs (1)

639-664: Use linear color for uniforms and avoid magic defaults

Uniforms should receive linear RGB, not sRGB. You already have srgb_to_linear(), but here RGB is divided by 255.0 directly. Also, the else branch hardcodes [1.0, 1.0, 1.0, 0.8]. Prefer deriving from a single shared default (see config DEFAULT_BORDER suggestion) and converting via srgb_to_linear for consistent visuals and future-proof defaults.

Apply:

-                border_color: if let Some(b) = project.background.border.as_ref() {
-                    [
-                        b.color[0] as f32 / 255.0,
-                        b.color[1] as f32 / 255.0,
-                        b.color[2] as f32 / 255.0,
-                        (b.opacity / 100.0).clamp(0.0, 1.0),
-                    ]
-                } else {
-                    [1.0, 1.0, 1.0, 0.8]
-                },
+                border_color: if let Some(b) = project.background.border.as_ref() {
+                    [
+                        srgb_to_linear(b.color[0]),
+                        srgb_to_linear(b.color[1]),
+                        srgb_to_linear(b.color[2]),
+                        (b.opacity / 100.0).clamp(0.0, 1.0),
+                    ]
+                } else {
+                    let d = cap_project::DEFAULT_BORDER;
+                    [
+                        srgb_to_linear(d.color[0]),
+                        srgb_to_linear(d.color[1]),
+                        srgb_to_linear(d.color[2]),
+                        (d.opacity / 100.0).clamp(0.0, 1.0),
+                    ]
+                },
crates/project/src/configuration.rs (2)

196-204: Introduce a shared default constant for border

A single exported DEFAULT_BORDER avoids duplicated literals across crates and keeps Rust/TS defaults in sync.

Apply:

 #[derive(Type, Serialize, Deserialize, Clone, Debug, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct BorderConfiguration {
     pub enabled: bool,
     pub width: f32,   // Border width in pixels
     pub color: Color, // Border color (RGB)
     pub opacity: f32, // Border opacity (0-100)
 }
+
+// Shared default used across codebases
+pub const DEFAULT_BORDER: BorderConfiguration = BorderConfiguration {
+    enabled: false,
+    width: 5.0,
+    color: [255, 255, 255],
+    opacity: 80.0,
+};

222-231: Reuse DEFAULT_BORDER in Default impl

Eliminate duplicate literals and ensure consistency.

 impl Default for BorderConfiguration {
     fn default() -> Self {
-        Self {
-            enabled: false,
-            width: 5.0,
-            color: [255, 255, 255], // White
-            opacity: 80.0,          // 80% opacity
-        }
+        DEFAULT_BORDER
     }
 }
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)

1513-1592: DRY up border defaults; define a single DEFAULT_BORDER on the UI side

The default object is repeated multiple times; centralize it to prevent drift and simplify updates.

Apply:

@@
 import {
   type BackgroundSource,
   type CameraShape,
   commands,
+  type BorderConfiguration,
   type SceneSegment,
   type StereoMode,
   type TimelineSegment,
   type ZoomSegment,
 } from "~/utils/tauri";
@@
 function BackgroundConfig(props: { scrollRef: HTMLDivElement }) {
@@
   const backgrounds: {
@@
   };
+
+  // UI-side default to mirror Rust DEFAULT_BORDER
+  const DEFAULT_BORDER: BorderConfiguration = {
+    enabled: false,
+    width: 5.0,
+    color: [255, 255, 255],
+    opacity: 80.0,
+  };
@@
-          const prev = project.background.border ?? {
-            enabled: false,
-            width: 5.0,
-            color: [255, 255, 255],
-            opacity: 80.0,
-          };
+          const prev = project.background.border ?? DEFAULT_BORDER;
@@
-            ...(project.background.border ?? {
-              enabled: true,
-              width: 5.0,
-              color: [255, 255, 255],
-              opacity: 80.0,
-            }),
+            ...(project.background.border ?? { ...DEFAULT_BORDER, enabled: true }),
             width: v[0],
           })
@@
-            ...(project.background.border ?? {
-              enabled: true,
-              width: 5.0,
-              color: [255, 255, 255],
-              opacity: 80.0,
-            }),
+            ...(project.background.border ?? { ...DEFAULT_BORDER, enabled: true }),
             color,
           })
@@
-            ...(project.background.border ?? {
-              enabled: true,
-              width: 5.0,
-              color: [255, 255, 255],
-              opacity: 80.0,
-            }),
+            ...(project.background.border ?? { ...DEFAULT_BORDER, enabled: true }),
             opacity: v[0],
           })
crates/rendering/src/shaders/composite-video-frame.wgsl (2)

57-58: Unused variable dist should be removed.

The dist variable is computed but never used elsewhere in the shader. Consider removing it to avoid confusion.

-    
-    let dist = sdf_rounded_rect(p - center, size, uniforms.rounding_px);

97-113: Consider optimizing border rendering logic.

The border rendering implementation looks correct, but there are a few potential improvements:

  1. The antialiasing range (-0.5, 0.5) is quite narrow and may cause aliasing on low-DPI displays
  2. Consider caching the border check result to avoid redundant SDF calculations
 if (uniforms.border_enabled > 0.0) {
     let border_outer_dist = sdf_rounded_rect(
         p - center,
         size + vec2<f32>(uniforms.border_width),
         uniforms.rounding_px + uniforms.border_width
     );
     let border_inner_dist = sdf_rounded_rect(p - center, size, uniforms.rounding_px);
 
     if (border_outer_dist <= 0.0 && border_inner_dist > 0.0) {
-        let inner_alpha = smoothstep(-0.5, 0.5, border_inner_dist);
-        let outer_alpha = 1.0 - smoothstep(-0.5, 0.5, border_outer_dist);
+        // Use 1.0 pixel for antialiasing on both edges for smoother borders
+        let inner_alpha = smoothstep(-1.0, 1.0, border_inner_dist);
+        let outer_alpha = 1.0 - smoothstep(-1.0, 1.0, border_outer_dist);
         let edge_alpha = inner_alpha * outer_alpha;
 
         let border_alpha = edge_alpha * uniforms.border_color.w;
         return vec4<f32>(uniforms.border_color.xyz, border_alpha);
     }
 }
crates/rendering/src/composite_frame.rs (2)

29-35: Consider documenting the padding fields' purpose.

The multiple padding fields (_padding0, _padding1, _padding1b, _padding2) ensure proper memory alignment for GPU buffers, but their purpose and alignment requirements aren't documented. Consider adding a comment explaining the alignment requirements.

     pub opacity: f32,
     pub border_enabled: f32,
     pub border_width: f32,
+    // Padding to ensure 16-byte alignment for the next vec4 (border_color)
     pub _padding0: f32,
     pub _padding1: [f32; 2],
     pub _padding1b: [f32; 2],
     pub border_color: [f32; 4],
+    // Padding to maintain struct alignment
     pub _padding2: [f32; 4],

56-62: Consider making border defaults configurable or documented.

The default border width of 5.0 pixels and white color with 0.8 alpha are hardcoded. These might not be suitable for all use cases.

Consider either:

  1. Making these values configurable through constants
  2. Adding documentation explaining the choice of defaults
+// Default border appearance when enabled
+const DEFAULT_BORDER_WIDTH: f32 = 5.0;
+const DEFAULT_BORDER_COLOR: [f32; 4] = [1.0, 1.0, 1.0, 0.8]; // White with 80% opacity
+
 impl Default for CompositeVideoFrameUniforms {
     fn default() -> Self {
         Self {
             // ... other fields ...
             border_enabled: 0.0,
-            border_width: 5.0,
+            border_width: DEFAULT_BORDER_WIDTH,
             _padding0: 0.0,
             _padding1: [0.0; 2],
             _padding1b: [0.0; 2],
-            border_color: [1.0, 1.0, 1.0, 0.8],
+            border_color: DEFAULT_BORDER_COLOR,
             _padding2: [0.0; 4],
         }
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce505f2 and 514e662.

📒 Files selected for processing (10)
  • apps/desktop/src/routes/editor/ConfigSidebar.tsx (3 hunks)
  • apps/desktop/src/utils/tauri.ts (1 hunks)
  • crates/project/src/configuration.rs (3 hunks)
  • crates/rendering-skia/src/bin/test_background.rs (5 hunks)
  • crates/rendering-skia/src/layers/background.rs (5 hunks)
  • crates/rendering-skia/src/layers/mod.rs (1 hunks)
  • crates/rendering/src/composite_frame.rs (3 hunks)
  • crates/rendering/src/layers/display.rs (1 hunks)
  • crates/rendering/src/lib.rs (3 hunks)
  • crates/rendering/src/shaders/composite-video-frame.wgsl (4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/tauri.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Never edit auto-generated IPC bindings file: tauri.ts

Files:

  • apps/desktop/src/utils/tauri.ts
apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app

Files:

  • apps/desktop/src/utils/tauri.ts
  • apps/desktop/src/routes/editor/ConfigSidebar.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable

Files:

  • apps/desktop/src/utils/tauri.ts
  • apps/desktop/src/routes/editor/ConfigSidebar.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use strict TypeScript and avoid any; prefer shared types from packages

Files:

  • apps/desktop/src/utils/tauri.ts
  • apps/desktop/src/routes/editor/ConfigSidebar.tsx
⏰ 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
  • GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
  • GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (12)
crates/rendering-skia/src/layers/mod.rs (1)

14-16: Uniforms now include an optional border — OK.

The added field matches usage downstream. Ensure cap_project::BorderConfiguration derives Clone + PartialEq so needs_update comparisons elsewhere compile and behave correctly.

crates/rendering-skia/src/layers/background.rs (2)

61-66: Tracking current/last border — good addition.

State mirrors background handling and enables cache invalidation on border changes.

Also applies to: 77-80


264-273: needs_update covers border and size — LGTM.

Make sure BorderConfiguration derives PartialEq to keep this comparison reliable.

crates/rendering/src/lib.rs (3)

772-779: LGTM: border disabled for camera overlay uniforms

Zeroing border fields here is correct; camera overlays shouldn’t inherit background border.


841-848: LGTM: border disabled for camera-only uniforms

Consistent with camera overlay path; no border in camera-only mode.


639-664: Struct layout validated: Rust CompositeVideoFrameUniforms and WGSL Uniforms match in field order, alignment, and total size.

crates/project/src/configuration.rs (2)

218-220: LGTM: optional border on BackgroundConfiguration

Serde default preserves backward compatibility; older projects won’t break.


244-245: LGTM: border disabled by default for compatibility

Defaulting to None is the right call for existing projects; UI enables and initializes as needed.

apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)

2494-2494: LGTM: bind color input value to current color

Keeps the native color picker in sync with state.

crates/rendering/src/shaders/composite-video-frame.wgsl (1)

17-21: LGTM! Border uniforms properly structured with appropriate padding.

The border-related uniforms are correctly added with proper padding to maintain alignment requirements for GPU buffers. The use of f32 for border_enabled as a boolean flag is appropriate for shader compatibility.

crates/rendering/src/composite_frame.rs (2)

78-80: LGTM! Clean implementation of buffer update method.

The write_to_buffer method provides a clean API for updating GPU uniforms in-place, which is more efficient than recreating buffers for dynamic updates.


29-35: Add a compile-time layout assertion for CompositeVideoFrameUniforms.
No existing tests validate its size or padding against the WGSL definition—insert a const _: () = assert!(std::mem::size_of::<CompositeVideoFrameUniforms>() == EXPECTED_SIZE); (and optionally align_of) immediately after the struct to guarantee Rust & WGSL stay in sync.

Comment on lines 115 to 117
if target_uv.x < 0.0 || target_uv.x > 1.0 || target_uv.y < 0.0 || target_uv.y > 1.0 {
return shadow_color;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Border doesn't properly composite with shadow.

When the border is enabled but the pixel is outside the border region (falls through to line 115), the shadow is returned directly without considering potential border-shadow blending at the edges. This could create a harsh transition between border and shadow.

Consider blending the border with the shadow for smoother transitions:

+    // Check if we're in the shadow region after border check
     if target_uv.x < 0.0 || target_uv.x > 1.0 || target_uv.y < 0.0 || target_uv.y > 1.0 {
+        // If border is enabled and we're in the outer border region, blend with shadow
+        if (uniforms.border_enabled > 0.0) {
+            let border_outer_dist = sdf_rounded_rect(
+                p - center,
+                size + vec2<f32>(uniforms.border_width),
+                uniforms.rounding_px + uniforms.border_width
+            );
+            
+            if (border_outer_dist <= 1.0 && border_outer_dist > -1.0) {
+                // We're at the edge of the border, blend with shadow
+                let border_fade = smoothstep(1.0, -1.0, border_outer_dist);
+                let border_color_with_alpha = vec4<f32>(uniforms.border_color.xyz, uniforms.border_color.w * border_fade);
+                return mix(shadow_color, border_color_with_alpha, border_color_with_alpha.a);
+            }
+        }
         return shadow_color;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if target_uv.x < 0.0 || target_uv.x > 1.0 || target_uv.y < 0.0 || target_uv.y > 1.0 {
return shadow_color;
}
// Check if we're in the shadow region after border check
if target_uv.x < 0.0 || target_uv.x > 1.0 || target_uv.y < 0.0 || target_uv.y > 1.0 {
// If border is enabled and we're in the outer border region, blend with shadow
if (uniforms.border_enabled > 0.0) {
let border_outer_dist = sdf_rounded_rect(
p - center,
size + vec2<f32>(uniforms.border_width),
uniforms.rounding_px + uniforms.border_width
);
if (border_outer_dist <= 1.0 && border_outer_dist > -1.0) {
// We're at the edge of the border, blend with shadow
let border_fade = smoothstep(1.0, -1.0, border_outer_dist);
let border_color_with_alpha = vec4<f32>(
uniforms.border_color.xyz,
uniforms.border_color.w * border_fade
);
return mix(shadow_color, border_color_with_alpha, border_color_with_alpha.a);
}
}
return shadow_color;
}
🤖 Prompt for AI Agents
In crates/rendering/src/shaders/composite-video-frame.wgsl around lines 115–117,
the shader returns shadow_color immediately for UVs outside [0,1], causing a
hard edge where a border should blend into the shadow; change this to compute a
blend between the border color and shadow based on distance to the border edge
(use the distance of target_uv to the nearest border boundary and a
smoothstep/feather value) so pixels just outside the border are mixed
(border*alpha + shadow*(1-alpha)) rather than replaced outright, ensuring the
border and shadow transition smoothly.

@richiemcilroy richiemcilroy merged commit 737c7ee into main Sep 8, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants