Improve project autosave and media path handling#369
Merged
webadderall merged 3 commits intomainfrom Apr 27, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR extends Electron IPC method signatures to include an optional Changes
Sequence DiagramsequenceDiagram
participant Renderer as Renderer (VideoEditor)
participant Preload as Preload API
participant IPC as IPC Handler
participant Manager as Project Manager
participant FS as File System
Renderer->>Renderer: saveProject(options)
activate Renderer
Renderer->>Preload: setCurrentVideoPath(path, {preserveProjectPath: true})
deactivate Renderer
activate Preload
Preload->>IPC: send('set-current-video-path', path, options)
deactivate Preload
activate IPC
IPC->>Manager: resolveApprovedLocalMediaPath(path)
deactivate IPC
activate Manager
Manager->>FS: fs.realpath(normalizedPath)
FS-->>Manager: realCanonicalPath
Manager->>Manager: collectApprovedLocalReadPaths(candidatePath)
Manager->>Manager: rememberApprovedLocalReadPath(candidatePath)
Note over Manager: Add normalized + realpath to approvedLocalReadPaths
Manager-->>IPC: {success, webcamPath}
deactivate Manager
activate IPC
alt preserveProjectPath = true
Note over IPC: Skip clearing active project
else preserveProjectPath = false
IPC->>Manager: setCurrentProjectPath(null)
end
IPC-->>Preload: result
deactivate IPC
activate Preload
Preload-->>Renderer: resolve(result)
deactivate Preload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/components/video-editor/VideoEditor.tsx (1)
2429-2486:⚠️ Potential issue | 🟠 MajorSerialize named saves with autosave as well.
saveProjectWithName()bypasses bothclearPendingProjectAutosave()andqueueProjectSave(). If a rename lands while the 1-second autosave is pending, the autosave can still fire with the stalecurrentProjectPath; once the main process has switched trust to the new path, that stale save falls back toshowSaveDialog()or writes against the old file unexpectedly.🛠️ Suggested fix
const saveProjectWithName = useCallback( async (projectName: string) => { + clearPendingProjectAutosave(); - const trimmedProjectName = projectName.trim(); - if (!trimmedProjectName) { - toast.error("Project name is required"); - return false; - } - - if (!currentSourcePath) { - toast.error("No video loaded"); - return false; - } - - try { - const projectData = - currentProjectSnapshot?.videoPath === currentSourcePath - ? currentProjectSnapshot - : createProjectData( - currentSourcePath, - currentPersistedEditorState, - lastSavedSnapshot?.projectId ?? null, - ); - const thumbnailDataUrl = await captureProjectThumbnail(); - const result = await window.electronAPI.saveProjectFileNamed( - projectData, - trimmedProjectName, - thumbnailDataUrl, - ); + return queueProjectSave(async () => { + const trimmedProjectName = projectName.trim(); + if (!trimmedProjectName) { + toast.error("Project name is required"); + return false; + } + + if (!currentSourcePath) { + toast.error("No video loaded"); + return false; + } + + try { + const projectData = + currentProjectSnapshot?.videoPath === currentSourcePath + ? currentProjectSnapshot + : createProjectData( + currentSourcePath, + currentPersistedEditorState, + lastSavedSnapshot?.projectId ?? null, + ); + const thumbnailDataUrl = await captureProjectThumbnail(); + const result = await window.electronAPI.saveProjectFileNamed( + projectData, + trimmedProjectName, + thumbnailDataUrl, + ); - if (result.canceled) { - toast.info("Project save canceled"); - return false; - } + if (result.canceled) { + toast.info("Project save canceled"); + return false; + } - if (!result.success) { - toast.error(result.message || "Failed to save project"); - return false; - } + if (!result.success) { + toast.error(result.message || "Failed to save project"); + return false; + } - if (result.path) { - setCurrentProjectPath(result.path); - } - setLastSavedSnapshot( - cloneStructured( - createProjectData( - projectData.videoPath, - projectData.editor, - result.projectId ?? projectData.projectId ?? null, - ), - ), - ); - await refreshProjectLibrary(); - toast.success(result.path ? `Project saved to ${result.path}` : "Project saved"); - return true; - } finally { - remountPreview(); - } + if (result.path) { + setCurrentProjectPath(result.path); + } + setLastSavedSnapshot( + cloneStructured( + createProjectData( + projectData.videoPath, + projectData.editor, + result.projectId ?? projectData.projectId ?? null, + ), + ), + ); + await refreshProjectLibrary(); + toast.success(result.path ? `Project saved to ${result.path}` : "Project saved"); + return true; + } finally { + remountPreview(); + } + }); }, [ captureProjectThumbnail, + clearPendingProjectAutosave, currentPersistedEditorState, currentProjectSnapshot, currentSourcePath, lastSavedSnapshot?.projectId, + queueProjectSave, refreshProjectLibrary, remountPreview, ], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 2429 - 2486, saveProjectWithName currently performs the save directly and can race with the autosave flow; update it to first clear any pending autosave by calling clearPendingProjectAutosave(), then perform the named save through the same serializer used by autosave (i.e. call/await queueProjectSave or otherwise enqueue the save operation instead of calling window.electronAPI.saveProjectFileNamed directly) so saves are serialized with autosave; ensure you still handle the returned result (result.success/result.path/result.canceled), update setCurrentProjectPath and setLastSavedSnapshot only after the queued save completes, and keep remountPreview() behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/electron-env.d.ts`:
- Around line 401-404: Update the setCurrentVideoPath declaration so its Promise
return type matches the IPC handler payload by including the webcamPath field in
addition to success; change the signature for setCurrentVideoPath to return a
Promise whose resolved value contains both success and webcamPath (webcamPath
can be optional if the handler sometimes omits it) so renderer callers get the
synced webcam path without casting.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2429-2486: saveProjectWithName currently performs the save
directly and can race with the autosave flow; update it to first clear any
pending autosave by calling clearPendingProjectAutosave(), then perform the
named save through the same serializer used by autosave (i.e. call/await
queueProjectSave or otherwise enqueue the save operation instead of calling
window.electronAPI.saveProjectFileNamed directly) so saves are serialized with
autosave; ensure you still handle the returned result
(result.success/result.path/result.canceled), update setCurrentProjectPath and
setLastSavedSnapshot only after the queued save completes, and keep
remountPreview() behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cb89a0fc-1798-478c-b662-141e148bb785
📒 Files selected for processing (8)
electron/electron-env.d.tselectron/ipc/project/manager.test.tselectron/ipc/project/manager.tselectron/ipc/register/project.tselectron/preload.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/editorPreferences.test.tssrc/lib/exporter/localMediaSource.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/lib/backgroundGradients.ts (1)
363-366: Consider exporting an immutable gradients list.
GRADIENTSis globally shared and mutable; accidentalpush/spliceby consumers would affect all users.💡 Proposed refactor
-export const GRADIENTS = [ +export const GRADIENTS = Object.freeze([ ...BLOB_GRADIENT_PRESETS.map(blobGradientPreviewCss), ...LINEAR_GRADIENTS, -]; +]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/backgroundGradients.ts` around lines 363 - 366, GRADIENTS is a shared mutable array — make it immutable so consumers can't push/splice it; replace the current export with an immutable version created from the mapped values (use a new array spread of ...BLOB_GRADIENT_PRESETS.map(blobGradientPreviewCss) and ...LINEAR_GRADIENTS) and freeze it / type it as ReadonlyArray (e.g., Object.freeze([...]) and/or annotate as ReadonlyArray<...>) so GRADIENTS, BLOB_GRADIENT_PRESETS, blobGradientPreviewCss and LINEAR_GRADIENTS remain safe from mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 105-110: getBackgroundTabForWallpaper now classifies any
linear-/radial-gradient as a gradient tab, but the component's local gradient
state (the gradient variable and its setter, e.g., setGradient) still only knows
about entries in GRADIENTS, so custom gradients show the wrong selected tile and
get overwritten; fix by, when getBackgroundTabForWallpaper returns the gradient
tab, synchronizing the component's gradient state to the actual wallpaper value
(store the raw gradient string or a sentinel like "custom-gradient") instead of
only matching GRADIENTS, and update the tile-selection logic to highlight a
"custom" tile when gradient state !== any GRADIENTS entry (or compare by exact
string match) so the panel correctly reflects and preserves
custom/extension-provided gradients.
In `@src/lib/backgroundGradients.ts`:
- Around line 74-92: parseRgbColor and parseHslColor should reject malformed
numeric channels instead of producing NaN-filled colors: after splitting and
parsing each channel (r,g,b,a / h,s,l,a) use Number.isFinite (or !Number.isNaN)
to validate parsed values and return null if any required channel is invalid,
only then clamp and return the color; update both functions (parseRgbColor,
parseHslColor) to perform this numeric validation so downstream toCssRgba()
receives either a fully-valid color or null (allow the default alpha when
missing but still validate it if present).
- Around line 161-173: blobCount can be fractional after clamping, causing the
for loop to behave inconsistently; normalize it to an integer (e.g., use
Math.round or Math.floor) right after computing it so the loop uses a
deterministic integer value. Update the declaration of blobCount (the symbol
blobCount created via clamp(Number(config.blobCount) || (...))) to wrap the
clamped result with an integer conversion like Math.round(...) (or
Math.floor(...) if you prefer downward rounding) so the subsequent for (let
index = 0; index < blobCount; index += 1) uses an integer count.
In `@src/lib/exporter/cssGradientBackground.ts`:
- Around line 244-251: The canvas state save/transform/restore block
(ctx.save(), ctx.translate(...), ctx.scale(...), createRadialGradient,
applyGradientStops, ctx.fillRect) can skip ctx.restore() if an exception occurs;
wrap the transform/gradient/fill sequence in a try/finally so ctx.restore() is
always called (i.e., call ctx.save() before the try and ctx.restore() in
finally) to prevent leaking transforms into the caller's fallback rendering,
referencing the ctx, layer, and applyGradientStops usage in this block.
---
Nitpick comments:
In `@src/lib/backgroundGradients.ts`:
- Around line 363-366: GRADIENTS is a shared mutable array — make it immutable
so consumers can't push/splice it; replace the current export with an immutable
version created from the mapped values (use a new array spread of
...BLOB_GRADIENT_PRESETS.map(blobGradientPreviewCss) and ...LINEAR_GRADIENTS)
and freeze it / type it as ReadonlyArray (e.g., Object.freeze([...]) and/or
annotate as ReadonlyArray<...>) so GRADIENTS, BLOB_GRADIENT_PRESETS,
blobGradientPreviewCss and LINEAR_GRADIENTS remain safe from mutation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5696daa1-8b76-4197-8c05-44565ba27d4d
⛔ Files ignored due to path filters (19)
public/wallpapers/energy-17.jpgis excluded by!**/*.jpgpublic/wallpapers/energy-19.jpgis excluded by!**/*.jpgpublic/wallpapers/glassmorphism-3.jpgis excluded by!**/*.jpgpublic/wallpapers/glassmorphism-4.jpgis excluded by!**/*.jpgpublic/wallpapers/ipad-17-dark.jpgis excluded by!**/*.jpgpublic/wallpapers/ipad-17-light.jpgis excluded by!**/*.jpgpublic/wallpapers/iridescent-9.jpgis excluded by!**/*.jpgpublic/wallpapers/midnight-8.jpgis excluded by!**/*.jpgpublic/wallpapers/sequoia-blue-orange.jpgis excluded by!**/*.jpgpublic/wallpapers/sequoia-blue.jpgis excluded by!**/*.jpgpublic/wallpapers/sonoma-clouds.jpgis excluded by!**/*.jpgpublic/wallpapers/sonoma-dark.jpgis excluded by!**/*.jpgpublic/wallpapers/sonoma-evening.jpgis excluded by!**/*.jpgpublic/wallpapers/sonoma-horizon.jpgis excluded by!**/*.jpgpublic/wallpapers/sonoma-light.jpgis excluded by!**/*.jpgpublic/wallpapers/tahoe-dark.jpgis excluded by!**/*.jpgpublic/wallpapers/tahoe-light.jpgis excluded by!**/*.jpgpublic/wallpapers/ventura-dark.jpgis excluded by!**/*.jpgpublic/wallpapers/ventura.jpgis excluded by!**/*.jpg
📒 Files selected for processing (9)
src/components/video-editor/SettingsPanel.tsxsrc/lib/backgroundGradients.test.tssrc/lib/backgroundGradients.tssrc/lib/exporter/cssGradientBackground.test.tssrc/lib/exporter/cssGradientBackground.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/wallpapers.test.tssrc/lib/wallpapers.ts
Comment on lines
+105
to
+110
| function getBackgroundTabForWallpaper(value: string): BackgroundTab { | ||
| if (GRADIENTS.includes(value)) { | ||
| if ( | ||
| GRADIENTS.includes(value) || | ||
| value.startsWith("linear-gradient") || | ||
| value.startsWith("radial-gradient") | ||
| ) { |
Contributor
There was a problem hiding this comment.
Don't route arbitrary CSS gradients to the preset tab without syncing the selected tile.
This now treats any linear-gradient... / radial-gradient... value as a gradient, but the local gradient state still only tracks members of GRADIENTS at Lines 971-973 and Lines 1082-1084. A project with a custom or extension-provided gradient will open with the first preset highlighted, so the panel misrepresents the actual background and the next click overwrites it.
Possible fix
- const [gradient, setGradient] = useState<string>(
- GRADIENTS.includes(selected) ? selected : GRADIENTS[0],
- );
+ const [gradient, setGradient] = useState<string>(
+ getBackgroundTabForWallpaper(selected) === "gradient" ? selected : GRADIENTS[0],
+ );- if (GRADIENTS.includes(selected)) {
+ if (getBackgroundTabForWallpaper(selected) === "gradient") {
setGradient(selected);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/SettingsPanel.tsx` around lines 105 - 110,
getBackgroundTabForWallpaper now classifies any linear-/radial-gradient as a
gradient tab, but the component's local gradient state (the gradient variable
and its setter, e.g., setGradient) still only knows about entries in GRADIENTS,
so custom gradients show the wrong selected tile and get overwritten; fix by,
when getBackgroundTabForWallpaper returns the gradient tab, synchronizing the
component's gradient state to the actual wallpaper value (store the raw gradient
string or a sentinel like "custom-gradient") instead of only matching GRADIENTS,
and update the tile-selection logic to highlight a "custom" tile when gradient
state !== any GRADIENTS entry (or compare by exact string match) so the panel
correctly reflects and preserves custom/extension-provided gradients.
Comment on lines
+74
to
+92
| function parseRgbColor(raw: string): RgbaColor | null { | ||
| const match = raw.match(/^rgba?\((.+)\)$/i); | ||
| if (!match) { | ||
| return null; | ||
| } | ||
|
|
||
| const parts = match[1].split(",").map((part) => part.trim()); | ||
| if (parts.length < 3) { | ||
| return null; | ||
| } | ||
|
|
||
| const [r, g, b, a = "1"] = parts; | ||
| return { | ||
| r: clamp(Number.parseFloat(r), 0, 255), | ||
| g: clamp(Number.parseFloat(g), 0, 255), | ||
| b: clamp(Number.parseFloat(b), 0, 255), | ||
| a: clamp(Number.parseFloat(a), 0, 1), | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle invalid numeric channels by returning null instead of NaN colors.
parseRgbColor / parseHslColor currently accept malformed numeric input and can return NaN channels. That propagates into toCssRgba() as invalid CSS (e.g., rgba(NaN, ...)) instead of using the fallback path.
💡 Proposed fix
function parseRgbColor(raw: string): RgbaColor | null {
const match = raw.match(/^rgba?\((.+)\)$/i);
if (!match) {
return null;
}
@@
const parts = match[1].split(",").map((part) => part.trim());
if (parts.length < 3) {
return null;
}
const [r, g, b, a = "1"] = parts;
+ const rNum = Number.parseFloat(r);
+ const gNum = Number.parseFloat(g);
+ const bNum = Number.parseFloat(b);
+ const aNum = Number.parseFloat(a);
+ if (![rNum, gNum, bNum, aNum].every(Number.isFinite)) {
+ return null;
+ }
return {
- r: clamp(Number.parseFloat(r), 0, 255),
- g: clamp(Number.parseFloat(g), 0, 255),
- b: clamp(Number.parseFloat(b), 0, 255),
- a: clamp(Number.parseFloat(a), 0, 1),
+ r: clamp(rNum, 0, 255),
+ g: clamp(gNum, 0, 255),
+ b: clamp(bNum, 0, 255),
+ a: clamp(aNum, 0, 1),
};
}
@@
function parseHslColor(raw: string): RgbaColor | null {
@@
const [hueRaw, saturationRaw, lightnessRaw, alphaRaw = "1"] = parts;
- const hue = ((((Number.parseFloat(hueRaw) % 360) + 360) % 360) / 360);
- const saturation = clamp(Number.parseFloat(saturationRaw) / 100, 0, 1);
- const lightness = clamp(Number.parseFloat(lightnessRaw) / 100, 0, 1);
- const alpha = clamp(Number.parseFloat(alphaRaw), 0, 1);
+ const hueNum = Number.parseFloat(hueRaw);
+ const saturationNum = Number.parseFloat(saturationRaw);
+ const lightnessNum = Number.parseFloat(lightnessRaw);
+ const alphaNum = Number.parseFloat(alphaRaw);
+ if (![hueNum, saturationNum, lightnessNum, alphaNum].every(Number.isFinite)) {
+ return null;
+ }
+ const hue = ((((hueNum % 360) + 360) % 360) / 360);
+ const saturation = clamp(saturationNum / 100, 0, 1);
+ const lightness = clamp(lightnessNum / 100, 0, 1);
+ const alpha = clamp(alphaNum, 0, 1);Also applies to: 104-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/backgroundGradients.ts` around lines 74 - 92, parseRgbColor and
parseHslColor should reject malformed numeric channels instead of producing
NaN-filled colors: after splitting and parsing each channel (r,g,b,a / h,s,l,a)
use Number.isFinite (or !Number.isNaN) to validate parsed values and return null
if any required channel is invalid, only then clamp and return the color; update
both functions (parseRgbColor, parseHslColor) to perform this numeric validation
so downstream toCssRgba() receives either a fully-valid color or null (allow the
default alpha when missing but still validate it if present).
Comment on lines
+161
to
+173
| const intensity = clamp(Number(config.intensity) || 0.85, 0, 1); | ||
| const blobCount = clamp(Number(config.blobCount) || (baseMode === "light" ? 6 : 7), 3, 10); | ||
|
|
||
| const rand = mulberry32(seed); | ||
| const pick = (index: number, fallback: string) => | ||
| String(colors[index % Math.max(1, colors.length)] || fallback); | ||
| const leadingAlpha = baseMode === "light" ? intensity * 0.3 : intensity * 0.7; | ||
| const trailingAlpha = baseMode === "light" ? intensity * 0.18 : intensity * 0.42; | ||
| const lobesPerBlob = 2; | ||
| const blobs: string[] = []; | ||
|
|
||
| for (let index = 0; index < blobCount; index += 1) { | ||
| for (let lobe = 0; lobe < lobesPerBlob; lobe += 1) { |
Contributor
There was a problem hiding this comment.
Normalize blobCount to an integer before iterating.
blobCount is clamped but can remain fractional. The loop then generates an implicit rounded-up/down count depending on value, which is hard to reason about.
💡 Proposed fix
- const blobCount = clamp(Number(config.blobCount) || (baseMode === "light" ? 6 : 7), 3, 10);
+ const blobCount = Math.floor(
+ clamp(Number(config.blobCount) || (baseMode === "light" ? 6 : 7), 3, 10),
+ );📝 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
| const intensity = clamp(Number(config.intensity) || 0.85, 0, 1); | |
| const blobCount = clamp(Number(config.blobCount) || (baseMode === "light" ? 6 : 7), 3, 10); | |
| const rand = mulberry32(seed); | |
| const pick = (index: number, fallback: string) => | |
| String(colors[index % Math.max(1, colors.length)] || fallback); | |
| const leadingAlpha = baseMode === "light" ? intensity * 0.3 : intensity * 0.7; | |
| const trailingAlpha = baseMode === "light" ? intensity * 0.18 : intensity * 0.42; | |
| const lobesPerBlob = 2; | |
| const blobs: string[] = []; | |
| for (let index = 0; index < blobCount; index += 1) { | |
| for (let lobe = 0; lobe < lobesPerBlob; lobe += 1) { | |
| const intensity = clamp(Number(config.intensity) || 0.85, 0, 1); | |
| const blobCount = Math.floor( | |
| clamp(Number(config.blobCount) || (baseMode === "light" ? 6 : 7), 3, 10), | |
| ); | |
| const rand = mulberry32(seed); | |
| const pick = (index: number, fallback: string) => | |
| String(colors[index % Math.max(1, colors.length)] || fallback); | |
| const leadingAlpha = baseMode === "light" ? intensity * 0.3 : intensity * 0.7; | |
| const trailingAlpha = baseMode === "light" ? intensity * 0.18 : intensity * 0.42; | |
| const lobesPerBlob = 2; | |
| const blobs: string[] = []; | |
| for (let index = 0; index < blobCount; index += 1) { | |
| for (let lobe = 0; lobe < lobesPerBlob; lobe += 1) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/backgroundGradients.ts` around lines 161 - 173, blobCount can be
fractional after clamping, causing the for loop to behave inconsistently;
normalize it to an integer (e.g., use Math.round or Math.floor) right after
computing it so the loop uses a deterministic integer value. Update the
declaration of blobCount (the symbol blobCount created via
clamp(Number(config.blobCount) || (...))) to wrap the clamped result with an
integer conversion like Math.round(...) (or Math.floor(...) if you prefer
downward rounding) so the subsequent for (let index = 0; index < blobCount;
index += 1) uses an integer count.
Comment on lines
+244
to
+251
| ctx.save(); | ||
| ctx.translate(layer.centerX, layer.centerY); | ||
| ctx.scale(layer.radiusX, layer.radiusY); | ||
| const gradient = ctx.createRadialGradient(0, 0, 0, 0, 0, 1); | ||
| applyGradientStops(gradient, layer.stops); | ||
| ctx.fillStyle = gradient; | ||
| ctx.fillRect(-2, -2, 4, 4); | ||
| ctx.restore(); |
Contributor
There was a problem hiding this comment.
Always restore the canvas state on the elliptical radial path.
If anything in this block throws, ctx.restore() is skipped and the caller's fallback fill runs on a transformed context. That can corrupt the rest of the background render.
Suggested fix
ctx.save();
- ctx.translate(layer.centerX, layer.centerY);
- ctx.scale(layer.radiusX, layer.radiusY);
- const gradient = ctx.createRadialGradient(0, 0, 0, 0, 0, 1);
- applyGradientStops(gradient, layer.stops);
- ctx.fillStyle = gradient;
- ctx.fillRect(-2, -2, 4, 4);
- ctx.restore();
+ try {
+ ctx.translate(layer.centerX, layer.centerY);
+ ctx.scale(layer.radiusX, layer.radiusY);
+ const gradient = ctx.createRadialGradient(0, 0, 0, 0, 0, 1);
+ applyGradientStops(gradient, layer.stops);
+ ctx.fillStyle = gradient;
+ ctx.fillRect(-2, -2, 4, 4);
+ } finally {
+ ctx.restore();
+ }📝 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
| ctx.save(); | |
| ctx.translate(layer.centerX, layer.centerY); | |
| ctx.scale(layer.radiusX, layer.radiusY); | |
| const gradient = ctx.createRadialGradient(0, 0, 0, 0, 0, 1); | |
| applyGradientStops(gradient, layer.stops); | |
| ctx.fillStyle = gradient; | |
| ctx.fillRect(-2, -2, 4, 4); | |
| ctx.restore(); | |
| ctx.save(); | |
| try { | |
| ctx.translate(layer.centerX, layer.centerY); | |
| ctx.scale(layer.radiusX, layer.radiusY); | |
| const gradient = ctx.createRadialGradient(0, 0, 0, 0, 0, 1); | |
| applyGradientStops(gradient, layer.stops); | |
| ctx.fillStyle = gradient; | |
| ctx.fillRect(-2, -2, 4, 4); | |
| } finally { | |
| ctx.restore(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/exporter/cssGradientBackground.ts` around lines 244 - 251, The canvas
state save/transform/restore block (ctx.save(), ctx.translate(...),
ctx.scale(...), createRadialGradient, applyGradientStops, ctx.fillRect) can skip
ctx.restore() if an exception occurs; wrap the transform/gradient/fill sequence
in a try/finally so ctx.restore() is always called (i.e., call ctx.save() before
the try and ctx.restore() in finally) to prevent leaking transforms into the
caller's fallback rendering, referencing the ctx, layer, and applyGradientStops
usage in this block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores