Fix: auto-set padding when background is selected#891
Fix: auto-set padding when background is selected#891Brendonovich merged 5 commits intoCapSoftware:mainfrom Enejivk:fix-editor-padding
Conversation
WalkthroughAdds a helper to initialize background.padding to 10 when selecting background source tabs or applying a wallpaper. Also replaces a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ConfigSidebar
participant ensurePad as ensurePaddingForBackground
participant Debounced as debouncedSetProject
participant Store as Project Store
User->>ConfigSidebar: select background tab / choose wallpaper
ConfigSidebar->>ensurePad: ensurePaddingForBackground(config)
ensurePad-->>ConfigSidebar: (sets padding=10 if was 0)
ConfigSidebar->>Debounced: debouncedSetProject(updatedConfig)
Debounced->>Store: apply updated config
Store-->>User: updated background applied
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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: 0
🧹 Nitpick comments (4)
packages/ui-solid/src/auto-imports.d.ts (1)
9-92: Generated icon globals: formatting-only churn; identifiers unchangedThe set of global icon identifiers and their default-type accessors remain identical; only quotes/semicolons/spacing changed. No semantic impact.
If feasible, align the unplugin-auto-import generator’s quote/semicolon settings to reduce diffs in future runs.
apps/desktop/src/utils/tauri.ts (3)
8-10: Nit: redundantreturn awaitin async functionsIn cases without a surrounding try/catch,
return TAURI_INVOKE(...)is equivalent and avoids an extra await. Not critical, especially in generated code.If you ever adjust the generator, consider this pattern:
-async setMicInput(label: string | null) : Promise<null> { - return await TAURI_INVOKE("set_mic_input", { label }); -}, +async setMicInput(label: string | null) : Promise<null> { + return TAURI_INVOKE("set_mic_input", { label }); +},
447-451: Importing Channel as a value is unnecessary here; prefer type-only import
Channelis only used in type position (TAURI_CHANNEL<FramesRendered>). Sinceinvokealready brings in the module as a value, this is minor, but you can avoid extra value imports.Apply if/when adjusting the generator:
import { invoke as TAURI_INVOKE, - Channel as TAURI_CHANNEL, + type Channel as TAURI_CHANNEL, } from "@tauri-apps/api/core";
328-330: Heads-up: duplicateBoundstype name in codebaseThis file exports
Bounds = { x, y, width, height }, whileapps/desktop/src/utils/box.tshasBounds = { size: XY; position: XY }(different shape). Not new, but it’s easy to import the wrong one by name.Consider:
- Aliasing on import at call sites (e.g.,
import type { Bounds as TauriBounds } ...) to avoid confusion, or- Renaming one of the types at the source (if feasible via generator settings) to make intent explicit (e.g.,
WindowBoundsvsLogicalBounds).
📜 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 (2)
apps/desktop/src/utils/tauri.ts(2 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/utils/tauri.ts (2)
apps/desktop/src/utils/box.ts (1)
Bounds(6-6)apps/desktop/src/store/captions.ts (1)
CaptionSettings(12-25)
🔇 Additional comments (5)
packages/ui-solid/src/auto-imports.d.ts (1)
7-7: LGTM: harmless formatting tweak on top-level exportRemoving the trailing semicolon from
export {}in a generated d.ts is non-functional. Safe to keep.apps/desktop/src/utils/tauri.ts (4)
8-260: Commands block: formatting-only; behavior unchangedThe API surface and invoke payloads are intact. These are generated; no runtime changes detected.
265-307: Events typing: callable object intersection matches the Proxy implementationThe change to a callable intersection shape aligns with the traps provided in
__makeEvents__. This improves type fidelity for bothevents.foo.listen(...)andevents.foo(win).listen(...).
475-477: Callable mapping type refinement looks correctThe signature
{ (handle: __WebviewWindow__): __EventObj__<T[K]>; }accurately captures the callable nature of the event accessors. Good improvement.
328-329: ensurePaddingForBackground is already implemented and invokedThe
ensurePaddingForBackgroundutility is defined and called inConfigSidebar.tsx, so the padding logic is present as intended:
- Definition: apps/desktop/src/routes/editor/ConfigSidebar.tsx lines 646–649
- Invoked in background‐type switch (color/gradient/wallpaper): line 659
- Invoked after setting a wallpaper path: line 1041
No missing padding logic detected.
| createPreset: async (preset: CreatePreset) => { | ||
| const config = { ...preset.config }; | ||
| // @ts-ignore we reeeally don't want the timeline in the preset | ||
| // @ts-expect-error we reeeally don't want the timeline in the preset |
There was a problem hiding this comment.
This was done by Biome automatically. Although it's outside of the changes on this PR gonna keep it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
653-663: Simplify handleBackgroundTabChange (all cases do the same).Since all four cases invoke ensurePaddingForBackground(), you can collapse the switch and future-proof it against new background types.
Apply:
-// this will run when user select a background tab -const handleBackgroundTabChange = (tab: BackgroundSource["type"]) => { - switch (tab) { - case "image": - case "color": - case "gradient": - case "wallpaper": - ensurePaddingForBackground(); - break; - } -}; +// this will run when user selects a background tab +const handleBackgroundTabChange = (_tab: BackgroundSource["type"]) => { + ensurePaddingForBackground(); +};
1041-1042: Avoid a second history entry when selecting wallpapers; make it atomic.Calling ensurePaddingForBackground() outside debouncedSetProject can create two separate history entries (padding change and wallpaper change). Move the ensure call inside debouncedSetProject’s batched update so the user gets a single undo step.
Apply this removal here:
- ensurePaddingForBackground();And update debouncedSetProject (outside this hunk) like this:
// Replace the existing debouncedSetProject with this version const debouncedSetProject = (wallpaperPath: string) => { const resumeHistory = projectHistory.pause(); queueMicrotask(() => { batch(() => { setProject("background", "source", { type: "wallpaper", path: wallpaperPath, } as const); ensurePaddingForBackground(); resumeHistory(); }); }); };
1148-1192: Image upload flow doesn’t currently trigger padding default; add ensure there.If the project is already on the “image” tab (no tab change), selecting/uploading an image won’t run handleBackgroundTabChange, so padding may remain 0 (contrary to the PR intent of applying to uploaded images). Call ensurePaddingForBackground() after setting the image source.
Add the call after setProject:
setProject("background", "source", { type: "image", path: fullPath, }); ensurePaddingForBackground();Note: If you also move ensurePaddingForBackground() into debouncedSetProject (as suggested for wallpapers), that won’t affect this code path; you still need the ensure call here.
📜 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 (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(3 hunks)apps/desktop/src/utils/createPresets.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
BackgroundSource(397-406)
🔇 Additional comments (3)
apps/desktop/src/utils/createPresets.ts (1)
29-30: Switch to ts-expect-error here is fine and intentional.Using // @ts-expect-error enforces the presence of the type error and documents intent to strip timeline from presets at runtime. No runtime impact.
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
645-651: Padding guard is correct and idempotent.The minimal guard prevents unnecessary writes and ensures the default is only applied when padding is exactly 0/undefined. Looks good.
803-807: Good: hook the padding initializer at the source-tab switch.Calling handleBackgroundTabChange before switching source guarantees the default padding is applied on tab change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (5)
645-650: Auto-padding helper is good; centralize the default and guard non-positive values.This looks correct and idempotent. Two small improvements:
- Replace the magic number 10 with a file-level constant to keep a single source of truth.
- Guard against any non-positive values (<= 0) to be future-proof.
Apply this diff:
- // set padding if background is selected - const ensurePaddingForBackground = () => { - if (project.background.padding === 0) - setProject("background", "padding", 10); - }; + // Auto-apply a default padding when a background is selected and padding is zero + const DEFAULT_BACKGROUND_PADDING = 10; + const ensurePaddingForBackground = () => { + if (project.background.padding <= 0) { + setProject("background", "padding", DEFAULT_BACKGROUND_PADDING); + } + };Follow-up note: If you want this applied universally whenever a wallpaper/image is chosen (even without switching tabs), wire this helper into those onChange handlers too (see comments below).
792-844: Batch ensurePadding + source-type switch to produce a single history entry (and avoid extra renders).Calling ensurePaddingForBackground outside the switch works, but it will create a separate update from the source-type change. Wrap both in Solid’s batch so they coalesce into one update/history entry.
Apply this diff:
onChange={(v) => { const tab = v as BackgroundSource["type"]; - ensurePaddingForBackground(); - switch (tab) { + batch(() => { + ensurePaddingForBackground(); + switch (tab) { case "image": { setProject("background", "source", { type: "image", path: project.background.source.type === "image" ? project.background.source.path : null, }); break; } case "color": { setProject("background", "source", { type: "color", value: project.background.source.type === "color" ? project.background.source.value : DEFAULT_GRADIENT_FROM, }); break; } case "gradient": { setProject("background", "source", { type: "gradient", from: project.background.source.type === "gradient" ? project.background.source.from : DEFAULT_GRADIENT_FROM, to: project.background.source.type === "gradient" ? project.background.source.to : DEFAULT_GRADIENT_TO, angle: project.background.source.type === "gradient" ? project.background.source.angle : 90, }); break; } case "wallpaper": { setProject("background", "source", { type: "wallpaper", path: project.background.source.type === "wallpaper" ? project.background.source.path : null, }); break; } - } + } + }); }}Optional: You may want to skip auto-padding when switching to the Image tab and there is no image selected yet (path is null). If that’s desired, gate ensurePaddingForBackground() behind something like: tab !== "image" || (project.background.source.type === "image" && project.background.source.path). I can draft that if you prefer.
1161-1179: Upload image path should also trigger auto-padding (same rationale as wallpapers).If the user is already on the Image tab (with padding 0) and uploads a file, we currently don’t auto-apply padding. Call ensurePaddingForBackground() (or batch it) right after setting the image path so uploaded images are covered too.
Proposed change (outside the changed hunk), inside the try block after setProject for the image:
setProject("background", "source", { type: "image", path: fullPath, }); ensurePaddingForBackground(); // or batch together with the setProject callAlternatively, adopt the same batching pattern as with wallpapers:
batch(() => { setProject("background", "source", { type: "image", path: fullPath }); if (project.background.padding <= 0) { setProject("background", "padding", DEFAULT_BACKGROUND_PADDING); } });
653-691: onMount wallpaper migration path doesn’t apply auto-padding.When a persisted wallpaper ID is resolved to a file URL on mount (radioGroupOnChange path), padding isn’t enforced. If you move the auto-padding into debouncedSetProject (as suggested), this path will automatically benefit. Otherwise, add ensurePaddingForBackground() in that flow too.
1018-1032: Wallpaper selection: batch auto-padding with the source update in debouncedSetProjectTo avoid a separate history entry—or worse, losing the padding change if history is paused—let’s move
ensurePaddingForBackground()inside the same microtask batch where we set the wallpaper. That way both padding and the new source land in one atomic update.• In
apps/desktop/src/routes/editor/ConfigSidebar.tsxaround line 1026, remove the standalone call toensurePaddingForBackground()in the photo‐URLonChangehandler:- debouncedSetProject(wallpaper.rawPath); - ensurePaddingForBackground(); + debouncedSetProject(wallpaper.rawPath);• In the
debouncedSetProjectdefinition (around line 744), prepend the padding check before setting the source:const debouncedSetProject = (wallpaperPath: string) => { const resumeHistory = projectHistory.pause(); queueMicrotask(() => { batch(() => { + if (project.background.padding <= 0) { + setProject("background", "padding", DEFAULT_BACKGROUND_PADDING); + } setProject("background", "source", { type: "wallpaper", path: wallpaperPath, } as const); resumeHistory(); }); }); };This ensures the padding and wallpaper-path updates are recorded together.
📜 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 (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(3 hunks)
|
Thanks for this! |
3 participants
Here’s a shorter, more direct version of your PR description:
Summary
Automatically sets padding to
10when a background (wallpaper, image, color, or gradient) is selected and current padding is0.Changes
Added
ensurePaddingForBackground()to apply default padding if none exists.Called it when:
Why
Ensures selected backgrounds are always visible, improving UX when padding was previously zero.
Apex_1755260578882.mp4
Summary by CodeRabbit
New Features
Chores