feat: Add border option to editor #947
Conversation
WalkthroughAdds a border feature to background rendering across UI, configuration, rendering pipeline, and shader. Introduces BorderConfiguration types, extends uniforms and serialization, updates GPU buffer handling, and adds editor controls. Renames a general settings field to snake_case. Shader gains logic to render an outline based on new uniforms. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Editor UI (ConfigSidebar)
participant Store as Project Config
participant Skia as Skia Layers
participant Rend as Rendering (Rust)
participant GPU as WGSL Shader
User->>UI: Enable Border / adjust width, color, opacity
UI->>Store: Update BackgroundConfiguration.border
Store-->>Skia: SkiaProjectUniforms includes border (optional)
Skia->>Rend: Prepare frame data with border params
Rend->>Rend: Build CompositeVideoFrameUniforms (border_* fields)
Rend->>GPU: Create/rebind uniforms buffer and draw
GPU->>GPU: If border_enabled>0, render outline before base sampling
GPU-->>User: Frame with border (if configured)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 fileThis 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 unitsLooks 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 cratesYou already implement
Defaulthere; 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_BORDERinstead of re-declaring the struct literal.crates/rendering/src/lib.rs (1)
639-651: Minor: simplifyborder_enabledcheckNon-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_andrequires Rust 1.70+. Use the existingmap_orif 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
_padding0and_padding1bfields compensate for WGSL’s implicit padding (after twof32s to alignvec2, and betweenvec2andvec4). 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>()matcheswgslreflection 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: Noneis permissible, but setting it toNonZeroU64::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_padding1andborder_color(to align vec4). Host-side Rust explicitly represents these as_padding0and_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.opacityso global fades affect the border too (if intended by UX; matches howbase_colorusesopacity).- 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 variabledist.
distis 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 unusedbg_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.
📒 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.tsapps/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.tsapps/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 moduleDropping 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-importandunplugin-iconsare pinned at^0.18.2and^0.19.2inpackages/ui-solid/package.jsonandapps/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 compatibilityAdding
#[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 intentKeeping
border: NoneinBackgroundConfiguration::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 BorderConfigurationThe 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 outputThe 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 LGTMSwitching to value import alias for
Channeland type-annotated value import forWebviewWindowis valid in TS 5.x and works with strict mode. No concerns.
481-488: Event factory shape: OK; ensures both global and window-scoped listenersThe callable proxy pattern lets you do
events.foo.listen(cb)orevents.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_updatealready checks border changesI’ve verified that
BackgroundLayer::needs_updatecompares the previous border state against the newuniforms.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_borderagainstuniforms.borderensures border edits invalidate the cache. Nice.
354-387: The scripts above will confirm whetherSkiaProjectUniformsnow includes aborderfield. Once we have that, we can update the tests accordingly.
124-131: The scripts will confirm the exact type ofBorderConfiguration.colorand how its defaults are defined, so we can validate whether the>> 8shifts 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, andborder_color: [1.0, 1.0, 1.0, 0.8]are sane defaults and match the editor defaults described. No issues.
| 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, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against negative or degenerate rect sizes for thick borders.
When border.width ≥ bounds.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.
| 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.
| self.uniforms_buffer = uniforms.to_buffer(device); | ||
|
|
||
| self.bind_group = Some(self.pipeline.bind_group( | ||
| device, | ||
| &self.uniforms_buffer, | ||
| &self.frame_texture_view, | ||
| )); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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], | ||
| } |
There was a problem hiding this comment.
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.
| 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.
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)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
2132-2136: Same path newline bug in ZoomSegmentConfigIdentical 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 updatesThe 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.
📒 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 — LGTMGood 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 — LGTMSetting 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 correctlyI’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 captureThe
custom_cursor_capture2flag is defined inapps/desktop/src/utils/tauri.tsbut your UI defaults still use the oldcustomCursorCapture2key 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.
| `${ | ||
| editorInstance.path | ||
| }/content/segments/segment-${segmentIndex()}/display.mp4`, | ||
| ); |
There was a problem hiding this comment.
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.
| `${ | |
| 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.
Introduces a new background border configuration feature in the
ConfigSidebarcomponent, with UI controls for configuring a border around the background, including toggling, width, color, and opacity.Demo:
Border.Demo.mp4
Summary by CodeRabbit