fix layout breaking when toggling border #1346
Conversation
ameer2468
changed the title
|
Warning Rate limit exceeded@ameer2468 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 0 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 (1)
WalkthroughReworks ConfigSidebar to inline former overlay UIs into KTabs.Content, adjusts tab content layout (padding/flex/min-h-0), refactors background wallpaper loading with debounce and URL resolution, moves Border controls into a KCollapsible with auto-scroll, and replaces ShadowSettings conditional rendering with KCollapsible-based animated content. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigSidebar
participant KTabs
participant BackgroundLoader
participant Toast
User->>ConfigSidebar: select tab / interact with controls
ConfigSidebar->>KTabs: set value = state.selectedTab
KTabs-->>ConfigSidebar: render corresponding KTabs.Content (inline controls)
alt Background tab selected
ConfigSidebar->>BackgroundLoader: applyWallpaper(path) [debounced]
BackgroundLoader-->>BackgroundLoader: resolve URL/path, load image
BackgroundLoader-->>ConfigSidebar: success / failure
BackgroundLoader->>Toast: show error (on failure)
end
Note right of ConfigSidebar: Opening Border KCollapsible triggers auto-scroll to reveal controls
sequenceDiagram
participant User
participant ShadowSettings
participant KCollapsible
User->>ShadowSettings: toggle Shadow section
ShadowSettings->>KCollapsible: set open = isOpen
KCollapsible-->>ShadowSettings: animate reveal
ShadowSettings->>ShadowSettings: setTimeout(200ms) -> scroll into view if needed
User->>ShadowSettings: adjust Size/Opacity/Blur sliders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.
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
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
apps/desktop/src/routes/editor/ui.tsx (7)
Field(25-47)Subfield(49-63)MenuItem(264-279)PopperContent(287-293)topSlideAnimateClasses(432-433)MenuItemList(295-309)Slider(65-147)apps/desktop/src/components/Toggle.tsx (1)
Toggle(37-50)apps/desktop/src/utils/tauri.ts (1)
StereoMode(473-473)
⏰ 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)
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)
647-879: Critical bug in deleteClipSegment validation logic.The handlers are properly implemented and called correctly from ConfigSidebar.tsx. However, there is a critical logic error in
deleteClipSegmentat line 111 ofapps/desktop/src/routes/editor/context.ts:!segment.recordingSegment === undefinedThis should be:
segment.recordingSegment === undefinedThe negation operator applied to
segment.recordingSegmentconverts it to a boolean before comparison, breaking the validation check.
♻️ Duplicate comments (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
1570-1609: Resume history after gradient drag.History is paused on line 1572 but never resumed. Once a user drags the gradient angle, the editor's history stays paused and no further state changes reach the undo stack.
Apply this diff to resume history on mouseup:
createRoot((dispose) => createEventListenerMap(window, { - mouseup: () => dispose(), + mouseup: () => { + resumeHistory(); + dispose(); + }, mousemove: (moveEvent) => { const rawNewAngle = Math.round(
🧹 Nitpick comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
1717-1781: Consider extracting default border config.The default border configuration
{ enabled: true, width: 5.0, color: [0, 0, 0], opacity: 50.0 }is repeated three times (lines 1725-1730, 1745-1750, 1764-1769). Extracting this to a constant would improve maintainability.Add this constant near the top of the BackgroundConfig function:
const DEFAULT_BORDER_CONFIG = { enabled: true, width: 5.0, color: [0, 0, 0] as [number, number, number], opacity: 50.0, };Then use it in the border controls:
onChange={(v) => setProject("background", "border", { - ...(project.background.border ?? { - enabled: true, - width: 5.0, - color: [0, 0, 0], - opacity: 50.0, - }), + ...(project.background.border ?? DEFAULT_BORDER_CONFIG), width: v[0], }) }
943-960: Consider simplifying wallpaper path resolution.The inline
radioGroupOnChangefunction definition followed by immediate invocation (lines 943-960) adds complexity. Consider extracting the path resolution logic directly:// Find the wallpaper with matching ID const wallpaper = loadedWallpapers.find((w) => w.id === path); if (wallpaper?.url) { try { const rawPath = decodeURIComponent( wallpaper.url.replace("file://", ""), ); debouncedSetProject(rawPath); } catch (err) { toast.error("Failed to set wallpaper"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(12 hunks)apps/desktop/src/routes/editor/ShadowSettings.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.
Files:
apps/desktop/src/routes/editor/ShadowSettings.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/ShadowSettings.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsx
🧬 Code graph analysis (2)
apps/desktop/src/routes/editor/ShadowSettings.tsx (1)
apps/desktop/src/routes/editor/ui.tsx (2)
Field(25-47)Slider(65-147)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
apps/desktop/src/routes/editor/CaptionsTab.tsx (1)
CaptionsTab(168-1145)apps/desktop/src/utils/tauri.ts (1)
ZoomSegment(497-497)apps/desktop/src/routes/editor/ui.tsx (2)
Field(25-47)Slider(65-147)
⏰ 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 (9)
apps/desktop/src/routes/editor/ShadowSettings.tsx (4)
1-1: LGTM!The import statement correctly aliases
CollapsibleasKCollapsible, maintaining consistency with the project's naming conventions for Kobalte components.
25-35: LGTM!The simplified timeout logic correctly waits 200ms for the collapsible animation to complete before scrolling to reveal the expanded content.
38-38: LGTM!Layout and typography adjustments are appropriate and consistent with the codebase styling patterns.
Also applies to: 44-44
52-84: LGTM!The KCollapsible implementation correctly uses Kobalte's animation classes and state management, maintaining consistency with the broader refactor across the editor UI.
apps/desktop/src/routes/editor/ConfigSidebar.tsx (5)
260-261: LGTM!Binding KTabs directly to
state.selectedTabprovides cleaner state management and aligns with the new tab control flow.
339-646: LGTM!Explicit KTabs.Content wrappers with consistent padding and flex layout classes improve the tab content structure and ensure proper overflow behavior.
1056-1056: LGTM!Explicit KTabs.Content wrapper with padding aligns with the tab content refactor pattern throughout the file.
1545-1566: LGTM!The gradient editor layout refactor improves visual hierarchy by separating interactive controls from preset selection, making the code more maintainable.
Also applies to: 1613-1648
1804-1845: LGTM!ShadowSettings component is consistently used in both BackgroundConfig and CameraConfig with proper prop handling and default values.
Also applies to: 2026-2067
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
1932-1937: Delay scroll to match KCollapsible animation timing.The scroll is triggered immediately when the border toggle changes, but the KCollapsible content takes 200ms to animate (line 1948). This can cause the newly expanded content to be partially off-screen or scroll to complete before the animation finishes.
Apply this diff to delay the scroll until after the animation completes:
<Toggle checked={project.background.border?.enabled ?? false} onChange={(enabled) => { const prev = project.background.border ?? { enabled: false, width: 5.0, color: [0, 0, 0], opacity: 50.0, }; - if (props.scrollRef) { - props.scrollRef.scrollTo({ - top: props.scrollRef.scrollHeight, - behavior: "smooth", - }); - } + setTimeout(() => { + if (enabled && props.scrollRef) { + props.scrollRef.scrollTo({ + top: props.scrollRef.scrollHeight, + behavior: "smooth", + }); + } + }, 200); setProject("background", "border", { ...prev, enabled, }); }} />Note: Also added a check to only scroll when enabling (not when collapsing).
1800-1839: Resume history after gradient drag.History is paused on line 1802 but the returned
resumeHistory()function is never called. Once a user drags the gradient angle, the editor's history remains paused and no further state changes reach the undo stack.Apply this diff to call
resumeHistory()in the mouseup handler:createRoot((dispose) => createEventListenerMap(window, { - mouseup: () => dispose(), + mouseup: () => { + resumeHistory(); + dispose(); + }, mousemove: (moveEvent) => {
🧹 Nitpick comments (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
632-632: Consider addingflex-colandgap-6for consistency.The hotkeys tab uses
"flex flex-1 p-4 min-h-0", while audio (line 339-341), cursor (458-460), and captions (641-643) tabs all use"flex flex-col flex-1 gap-6 p-4 min-h-0". Although hotkeys has minimal content currently, applying consistent classes would maintain uniformity across tabs.Apply this diff for consistency:
- <KTabs.Content value="hotkeys" class="flex flex-1 p-4 min-h-0"> + <KTabs.Content value="hotkeys" class="flex flex-col flex-1 gap-6 p-4 min-h-0">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.
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
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
apps/desktop/src/routes/editor/CaptionsTab.tsx (1)
CaptionsTab(168-1145)apps/desktop/src/utils/tauri.ts (2)
ZoomSegment(497-497)commands(7-292)apps/desktop/src/routes/editor/ui.tsx (3)
EditorButton(353-418)Field(25-47)Slider(65-147)
⏰ 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 (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
1947-2011: LGTM: Border controls KCollapsible implementation.The KCollapsible implementation for border controls is well-structured with proper animation classes, consistent Field/Slider/RgbInput patterns, and appropriate default value handling. The animation timing aligns with the recommended scroll delay.
Summary by CodeRabbit
New Features
Bug Fixes