renderer: add transparent background for gif exporting#1306
renderer: add transparent background for gif exporting#1306Brendonovich merged 4 commits intomainfrom
Conversation
|
Warning Rate limit exceeded@Brendonovich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds alpha/transparent color support end-to-end: Tooltip prop made optional; UI color inputs and swatches handle RGBA and explicit transparent swatch; Tauri and Rust configs include alpha; rendering pipeline, shader, and CLI updated to propagate premultiplied alpha and clear to transparent. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (main.rs)
participant Project as Project Config
participant Render as Render Pipeline
participant Channel as Frame Channel
participant Output as Output File
CLI->>Project: Load project-config.json
Project-->>CLI: ProjectRecordingsMeta
CLI->>Render: Spawn render task (alpha-aware)
Render->>Render: Apply BackgroundSource (with alpha, premultiply)
activate Render
Render->>Channel: Send rendered frame (RGBA premultiplied)
Channel-->>CLI: Receive frame
deactivate Render
CLI->>Render: Abort render task
CLI->>Output: Save frame as PNG/JPEG/RAW
Output-->>CLI: File saved
sequenceDiagram
participant UI as ConfigSidebar
participant ColorInput as Color Input
participant HexToRgb as hexToRgb()
participant State as Background State
UI->>ColorInput: User selects color (e.g. #00000000)
ColorInput->>HexToRgb: Parse hex to [r,g,b,(a)]
HexToRgb-->>ColorInput: Return [r,g,b,a?]
ColorInput->>State: Update background with color + alpha
State-->>UI: Render swatch (checkerboard for transparent)
rect rgba(200,150,255,0.12)
Note over UI,State: Alpha preserved through UI → Tauri → Rendering
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 5
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/ExportDialog.tsx (1)
951-963: Remove non-null assertion that defeats optional chaining.Line 962 uses
meta().sharing?.link!with both optional chaining and a non-null assertion. Ifsharingorlinkis undefined, this will cause a runtime error. The non-null assertion defeats the purpose of the optional chaining.Apply this diff to properly handle the optional value:
<Button onClick={() => { setCopyPressed(true); setTimeout(() => { setCopyPressed(false); }, 2000); - navigator.clipboard.writeText(meta().sharing?.link!); + const link = meta().sharing?.link; + if (link) navigator.clipboard.writeText(link); }} variant="dark" class="flex gap-2 justify-center items-center" >
🧹 Nitpick comments (6)
apps/desktop/src/components/Tooltip.tsx (1)
31-32: Consider conditional rendering for optional content.When
contentisundefined, thespanelement still renders withundefinedinside it. While JSX handles this gracefully, it's cleaner to conditionally render the content.Apply this diff to conditionally render the content:
<KTooltip.Content class="z-50 px-1.5 flex items-center py-1 text-xs border border-gray-3 bg-gray-12 text-gray-1 rounded-md shadow-lg duration-100 animate-in fade-in slide-in-from-top-1 min-w-6 gap-1.5 text-center"> - <span>{props.content}</span> + {props.content && <span>{props.content}</span>} {props.kbd && props.kbd.length > 0 && (apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
2883-2903: LGTM: RGBA support in hexToRgb is correctly implemented.The enhanced function now properly handles both 6-digit RGB and 8-digit RGBA hex values, consistently returning a 4-element tuple with alpha defaulting to 255.
Consider updating the return type to be non-nullable for the success case:
-function hexToRgb(hex: string): [number, number, number, number] | null { +function hexToRgb(hex: string): [number, number, number, number] | null {The current type is already correct, but you might consider a discriminated union or separate functions if the null case needs distinct handling throughout the codebase.
2795-2869: Extract repeated RGBA-to-RGB conversion logic.The same pattern of checking
value.length === 4and extracting RGB components is repeated in three handlers (onChange, onInput, onBlur). SincehexToRgbnow always returns a 4-element tuple, this can be simplified.Add a helper function to the component:
function RgbInput(props: { value: [number, number, number]; onChange: (value: [number, number, number]) => void; }) { // Helper to extract RGB from RGBA tuple const extractRgb = (rgba: [number, number, number, number] | null): [number, number, number] | null => { if (!rgba) return null; const [r, g, b] = rgba; return [r, g, b]; }; const [text, setText] = createWritableMemo(() => rgbToHex(props.value)); let prevHex = rgbToHex(props.value); let colorInput!: HTMLInputElement; return ( <div class="flex flex-row items-center gap-[0.75rem] relative"> {/* ... */} <input ref={colorInput} type="color" class="absolute left-0 bottom-0 w-[3rem] opacity-0" value={rgbToHex(props.value)} onChange={(e) => { const rgb = extractRgb(hexToRgb(e.target.value)); if (rgb) props.onChange(rgb); }} /> <TextInput class="w-[4.60rem] p-[0.375rem] text-gray-12 text-[13px] border rounded-[0.5rem] bg-gray-1 outline-none focus:ring-1 transition-shadows duration-200 focus:ring-gray-500 focus:ring-offset-1 focus:ring-offset-gray-200" value={text()} onFocus={() => { prevHex = rgbToHex(props.value); }} onInput={(e) => { setText(e.currentTarget.value); const rgb = extractRgb(hexToRgb(e.target.value)); if (rgb) props.onChange(rgb); }} onBlur={(e) => { const rgb = extractRgb(hexToRgb(e.target.value)); if (rgb) { props.onChange(rgb); } else { setText(prevHex); const fallbackRgb = extractRgb(hexToRgb(text())); if (fallbackRgb) props.onChange(fallbackRgb); } }} /> </div> ); }crates/rendering/src/main.rs (3)
170-183: Clarify FPS parameter intent.Line 178 sets
fps: 1with a comment "Only render 1 frame", butfps: 1means 1 frame per second, not limiting to 1 frame total. The actual frame count is(fps * duration).ceil(), so this could queue multiple frames before the task is aborted at line 196.The current approach works because you abort after receiving the first frame, but consider clarifying the comment:
- 1, // Only render 1 frame + 30, // FPS (we only use the first frame)Or if you want to be more explicit about rendering just one frame, you could modify the rendering logic to respect a frame limit, but that would require changes to
render_video_to_channel.
195-196: Consider graceful cancellation instead of abort.Using
abort()forcefully terminates the rendering task, which could leave resources (decoders, GPU contexts) in an inconsistent state. While Tokio's abort is generally safe, a cancellation token would allow for graceful cleanup.Consider adding a cancellation mechanism:
use tokio_util::sync::CancellationToken; // In main: let cancel_token = CancellationToken::new(); let cancel_token_clone = cancel_token.clone(); let render_task = tokio::task::spawn(async move { // Pass cancel_token to render_video_to_channel if it supports cancellation // Or check cancel_token periodically in the render loop }); // After receiving first frame: cancel_token.cancel(); let _ = render_task.await; // Wait for graceful shutdownAlternatively, if you don't want to modify
render_video_to_channel, the current abort approach is acceptable but should document that resources are forcefully released.
249-253: Document or remove padding from raw output format.The
save_as_rawfunction savesframe.datadirectly, which includes padding bytes at the end of each row (due topadded_bytes_per_row). Unlike PNG and JPEG formats which strip padding, this may not be what users expect for "raw RGBA data."Consider either:
- Stripping padding like PNG/JPEG do:
fn save_as_raw(frame: &RenderedFrame, output_path: &PathBuf) -> Result<()> { - // Save raw RGBA data - std::fs::write(output_path, &frame.data).context("Failed to save raw frame data")?; + // Save raw RGBA data (without padding) + let raw_data: Vec<u8> = frame + .data + .chunks(frame.padded_bytes_per_row as usize) + .flat_map(|row| row[0..(frame.width * 4) as usize].to_vec()) + .collect(); + std::fs::write(output_path, raw_data).context("Failed to save raw frame data")?; Ok(()) }
- Or document that padding is included:
fn save_as_raw(frame: &RenderedFrame, output_path: &PathBuf) -> Result<()> { - // Save raw RGBA data + // Save raw RGBA data (includes padding bytes at end of each row for GPU alignment) std::fs::write(output_path, &frame.data).context("Failed to save raw frame data")?; Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
apps/desktop/src/components/Tooltip.tsx(1 hunks)apps/desktop/src/routes/editor/ConfigSidebar.tsx(6 hunks)apps/desktop/src/routes/editor/ExportDialog.tsx(8 hunks)apps/desktop/src/routes/editor/Player.tsx(2 hunks)apps/desktop/src/utils/tauri.ts(1 hunks)crates/project/src/configuration.rs(2 hunks)crates/rendering/Cargo.toml(3 hunks)crates/rendering/src/layers/background.rs(3 hunks)crates/rendering/src/lib.rs(1 hunks)crates/rendering/src/main.rs(1 hunks)crates/rendering/src/shaders/gradient-or-color.wgsl(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/editor/Player.tsxapps/desktop/src/components/Tooltip.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/ExportDialog.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/editor/Player.tsxapps/desktop/src/components/Tooltip.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/ExportDialog.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/editor/Player.tsxapps/desktop/src/components/Tooltip.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/ExportDialog.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsx
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/project/src/configuration.rscrates/rendering/src/lib.rscrates/rendering/src/main.rscrates/rendering/src/layers/background.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/project/src/configuration.rscrates/rendering/src/lib.rscrates/rendering/src/main.rscrates/rendering/src/shaders/gradient-or-color.wgslcrates/rendering/src/layers/background.rs
**/tauri.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated files named
tauri.ts.NEVER EDIT auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
🧬 Code graph analysis (3)
crates/rendering/src/main.rs (4)
crates/project/src/configuration.rs (3)
new(69-71)from(169-174)from(178-180)crates/project/src/meta.rs (2)
load_for_project(147-153)studio_meta(194-199)crates/rendering/src/lib.rs (7)
new(63-133)new(307-350)new(629-974)new(990-995)new(1042-1052)new(1195-1228)render_video_to_channel(186-268)crates/export/src/mp4.rs (1)
frame(183-191)
apps/desktop/src/routes/editor/ExportDialog.tsx (4)
apps/desktop/src/components/Tooltip.tsx (1)
Tooltip(23-47)packages/ui-solid/src/Button.tsx (1)
Button(40-50)apps/desktop/src/routes/editor/Header.tsx (1)
RESOLUTION_OPTIONS(34-38)apps/desktop/src/routes/editor/context.ts (1)
meta(350-352)
crates/rendering/src/layers/background.rs (2)
apps/desktop/src/utils/tauri.ts (1)
BackgroundSource(357-357)crates/rendering/src/lib.rs (1)
srgb_to_linear(1432-1439)
⏰ 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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (16)
crates/project/src/configuration.rs (1)
34-35: LGTM! Alpha field properly added with sensible default.The alpha field is correctly added with a serde default of
u8::MAX(255), representing fully opaque colors. This maintains backward compatibility for existing configurations that don't have the alpha field.Also applies to: 49-51
apps/desktop/src/utils/tauri.ts (1)
357-357: Auto-generated type correctly reflects Rust changes.This file is auto-generated by tauri-specta (as noted in line 2), and the BackgroundSource type update correctly reflects the alpha field addition from the Rust configuration. No manual edits needed.
crates/rendering/src/lib.rs (1)
1146-1146: LGTM! Clear operation correctly updated for transparency support.Changing the initial clear operation from
BLACKtoTRANSPARENTis essential for transparent background support, allowing the background layer to properly control alpha values for formats like GIF with transparency.apps/desktop/src/routes/editor/Player.tsx (1)
332-342: LGTM! Checkerboard grid effectively visualizes transparency.The checkerboard background pattern provides clear visual feedback for transparent areas in the canvas, which is essential for the transparency support feature.
crates/rendering/src/shaders/gradient-or-color.wgsl (1)
23-23: LGTM! Shader correctly propagates alpha channel.The shader now properly blends the full RGBA uniforms including alpha, enabling transparent colors and gradients. This aligns with the premultiplied alpha approach used in the rendering pipeline.
crates/rendering/src/layers/background.rs (2)
36-40: LGTM! Alpha channel properly propagated from configuration.The color conversion correctly normalizes the alpha value from 0-255 to 0.0-1.0 and includes it in the color array, enabling transparency support.
437-448: LGTM! Premultiplied alpha correctly implemented.The uniforms properly premultiply RGB components with alpha for both Color and Gradient backgrounds. This is the correct approach for blending in the shader with alpha support.
Also applies to: 453-459
apps/desktop/src/routes/editor/ExportDialog.tsx (4)
124-132: LGTM: Transparent background detection is correctly implemented.The function properly checks for a color background source with an alpha channel value below 255 (fully opaque).
134-151: LGTM: Settings persistence and derived properties are well-implemented.The use of
makePersistedfor the underlying settings andmergePropsto compute derived values (forcing GIF when MP4 isn't compatible with transparent backgrounds) is a clean reactive pattern.
553-564: LGTM: Format disable logic is correctly implemented.The
disabledReasonfunction properly returns appropriate messages for incompatible format/export combinations.
567-614: LGTM: Tooltip integration for format options is well-implemented.The wrapping of format buttons in
Tooltipcomponents with dynamic disable reasons provides good user feedback. The conditional tooltip display and button disabling based ondisabledReason()is clean.apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
116-116: LGTM: Transparent color option added correctly.The fully transparent color
#00000000is a valid addition to the color palette.
1510-1515: LGTM: Transparent color visualization is well-implemented.The checkerboard pattern using a data URL SVG for the transparent color provides good visual feedback to users.
crates/rendering/src/main.rs (3)
13-54: LGTM: CLI structure is well-designed.The
Argsstruct andOutputFormatenum provide a clear interface for the rendering CLI tool with appropriate defaults and documentation.
63-89: LGTM: Project loading is correctly implemented.The loading sequence properly handles errors with context and creates the necessary shared structures for rendering.
209-247: LGTM: PNG and JPEG save functions correctly handle frame data.Both functions properly account for row padding and handle the RGBA-to-RGB conversion for JPEG format as needed.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
1140-1151: Fix invalid CSS when fallback uses hexToRgb (array has alpha=255)With hexToRgb now returning [r,g,b,a], the fallback here can yield rgb(r,g,b,255), which is invalid CSS and breaks the color dot. Slice to 3 channels and join explicitly.
Apply this diff:
- const backgroundColor = - project.background.source.type === "color" - ? project.background.source.value - : hexToRgb(BACKGROUND_COLORS[9]); + const backgroundColor: [number, number, number] = + project.background.source.type === "color" + ? project.background.source.value + : (hexToRgb(BACKGROUND_COLORS[9])!.slice(0, 3) as [number, number, number]); ... - "background-color": `rgb(${backgroundColor})`, + "background-color": `rgb(${backgroundColor.join(",")})`,
♻️ Duplicate comments (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
1471-1487: Deduplicate RGBA → BackgroundSource logic and omit alpha when 255This block is correct, but the pattern appears elsewhere and always sets alpha, even when 255 (no-op). Extract a helper and skip alpha=255 for cleaner state.
Apply this helper near hexToRgb:
+function createColorSource( + rgba: [number, number, number, number], +): Extract<BackgroundSource, { type: "color" }> { + const [r, g, b, a] = rgba; + return a < 255 ? { type: "color", value: [r, g, b], alpha: a } : { type: "color", value: [r, g, b] }; +}Then simplify here:
- const rgbValue = hexToRgb(color); + const rgbValue = hexToRgb(color); if (!rgbValue) return; - const [r, g, b, a] = rgbValue; - backgrounds.color = { type: "color", value: [r, g, b], alpha: a }; + backgrounds.color = createColorSource(rgbValue); setProject("background", "source", backgrounds.color);
🧹 Nitpick comments (5)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (5)
2777-2782: Prefer e.currentTarget for type-safe event access in SolidUse currentTarget to avoid EventTarget typing pitfalls.
Apply this diff:
- const value = hexToRgb(e.target.value); + const value = hexToRgb(e.currentTarget.value);
2794-2798: Same here: use currentTargetKeeps typings strict and consistent.
Apply this diff:
- const value = hexToRgb(e.target.value); + const value = hexToRgb(e.currentTarget.value);
2801-2812: And here: use currentTarget; fallback path looks goodGuarding and reverting to prevHex is solid. Just switch to currentTarget.
Apply this diff:
- const value = hexToRgb(e.target.value); + const value = hexToRgb(e.currentTarget.value);
2826-2847: hexToRgb: solid baseline; consider shorthand support and CSS helperThe RGBA tuple with default a=255 is good. Consider:
- Add shorthand hex (#RGB/#RGBA) for robustness.
- Add a small toCssColor([r,g,b,a]) helper to centralize rgb/rgba string generation and avoid future 3-vs-4 channel bugs.
Example helper:
function toCssColor([r, g, b, a = 255]: [number, number, number, number?]) { return a === 255 ? `rgb(${r}, ${g}, ${b})` : `rgba(${r}, ${g}, ${b}, ${(a / 255).toFixed(3)})`; }You can then use toCssColor in swatches/previews where alpha may appear.
2848-2848: Rename constant: spelling nit and reuseTypo: CHECKERD → CHECKERED. Rename for clarity and update usages.
Apply this diff:
-const CHECKERD_BUTTON_BACKGROUND = `url("data:image/svg+xml,%3Csvg width='16' height='16' xmlns='http://www.w3.org/2000/svg'%3E%3Crect width='8' height='8' fill='%23a0a0a0'/%3E%3Crect x='8' y='8' width='8' height='8' fill='%23a0a0a0'/%3E%3C/svg%3E")`; +const CHECKERED_BUTTON_BACKGROUND = `url("data:image/svg+xml,%3Csvg width='16' height='16' xmlns='http://www.w3.org/2000/svg'%3E%3Crect width='8' height='8' fill='%23a0a0a0'/%3E%3Crect x='8' y='8' width='8' height='8' fill='%23a0a0a0'/%3E%3C/svg%3E")`;And update reference:
- ? CHECKERD_BUTTON_BACKGROUND + ? CHECKERED_BUTTON_BACKGROUND
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/editor/ConfigSidebar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/editor/ConfigSidebar.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
116-116: Add transparent option: LGTMThe explicit "#00000000" entry makes transparency discoverable in the palette. No issues.
1492-1497: Nice UX for transparent swatchCheckerboard fallback for "#00000000" is a good touch and aligns with expectations.
Brendonovich
changed the title
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/ExportDialog.tsx (1)
951-962: Inconsistent optional chaining with non-null assertion.Line 962 uses both optional chaining (
?.) and non-null assertion (!), which is contradictory:navigator.clipboard.writeText(meta().sharing?.link!);The optional chaining acknowledges
sharingmight be undefined, but the non-null assertion assumeslinkis definitely defined. This could still throw at runtime ifsharingis undefined.Apply this fix:
-navigator.clipboard.writeText(meta().sharing?.link!); +const link = meta().sharing?.link; +if (link) navigator.clipboard.writeText(link);Or, if you're confident
sharingexists at this point:-navigator.clipboard.writeText(meta().sharing?.link!); +navigator.clipboard.writeText(meta().sharing!.link);
🧹 Nitpick comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
470-488: Consider only setting alpha when less than 255.The code always sets the
alphafield, even when the color is fully opaque (alpha = 255). SincehexToRgbnow always returns a 4-element tuple with alpha defaulting to 255, you could optimize by only including thealphafield when it's less than 255:const rgbValue = hexToRgb(color); if (!rgbValue) return; const [r, g, b, a] = rgbValue; backgrounds.color = { type: "color", value: [r, g, b], - alpha: a, + ...(a < 255 ? { alpha: a } : {}), };This keeps the data model cleaner by omitting redundant information.
apps/desktop/src/routes/editor/ExportDialog.tsx (1)
590-604: Use idiomatic membership test for FPS validation.While the logic is correct, using
.every(v => v.value !== settings.fps)to check if a value is NOT in an array is non-idiomatic. The more readable approach is!array.some(v => v.value === settings.fps):if ( option.value === "Gif" && - GIF_FPS_OPTIONS.every( - (v) => v.value !== settings.fps, + !GIF_FPS_OPTIONS.some( + (v) => v.value === settings.fps, ) ) newSettings.fps = 15; if ( option.value === "Mp4" && - FPS_OPTIONS.every( - (v) => v.value !== settings.fps, + !FPS_OPTIONS.some( + (v) => v.value === settings.fps, ) ) newSettings.fps = 30;This makes the intent clearer: "if the current FPS is not in the valid options for this format."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(5 hunks)apps/desktop/src/routes/editor/ExportDialog.tsx(8 hunks)apps/desktop/src/routes/editor/Player.tsx(2 hunks)crates/rendering/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/editor/Player.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/editor/ExportDialog.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/editor/ExportDialog.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/editor/ExportDialog.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/ExportDialog.tsx (3)
apps/desktop/src/components/Tooltip.tsx (1)
Tooltip(23-47)packages/ui-solid/src/Button.tsx (1)
Button(40-50)apps/desktop/src/routes/editor/Header.tsx (1)
RESOLUTION_OPTIONS(34-38)
⏰ 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 (9)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (5)
116-116: LGTM! Transparent color option added.The explicit transparent color option (#00000000) enables alpha-aware background selection for GIF exports with transparency.
492-497: LGTM! Checkerboard pattern for transparent colors.The conditional styling correctly shows a checkerboard pattern for the transparent color swatch, providing clear visual feedback to users.
2777-2812: LGTM! Correct extraction of RGB from RGBA.The RgbInput handlers correctly extract only the RGB values from the 4-element RGBA tuple returned by
hexToRgb, which is appropriate since this component only manages RGB color inputs.
2826-2846: LGTM! Enhanced hexToRgb with RGBA support.The function now correctly handles both 6-digit (RGB) and 8-digit (RGBA) hex colors, always returning a consistent 4-element tuple with alpha defaulting to 255 for opaque colors. This simplifies downstream handling.
2848-2848: LGTM! Standard transparency indicator pattern.The checkerboard SVG data URI is a standard visual pattern for indicating transparent colors in UI.
apps/desktop/src/routes/editor/ExportDialog.tsx (4)
19-19: LGTM! Necessary imports for new features.The
mergePropsandTooltipimports support the new transparency-aware format derivation and disabled-state messaging.Also applies to: 28-28
124-132: LGTM! Transparent background detection logic.The helper correctly identifies when a transparent background is configured, which is necessary for enforcing format constraints (MP4 doesn't support alpha channels).
134-151: LGTM! Derived settings enforce format constraints.The
mergePropsapproach correctly overrides the format to GIF when a transparent background is detected and MP4 is selected, ensuring the export format matches the project's transparency requirements.
553-564: LGTM! Clear disabled-state reasoning.The
disabledReasonhelper provides contextual explanations for when format options are unavailable, improving user experience through the Tooltip.
1 participant
Summary by CodeRabbit
New Features
Improvements