refactor: update images manager #1784

spartan-vutrannguyen · 2025-04-15T11:18:50Z

Description

Since NextJS allow user put images inside app folder, we should scan app folder to get images too.

Related Issues

Type of Change

Testing

Screenshots (if applicable)

Additional Notes

Important

Refactor image management to scan 'app' directory and update components for improved image handling using ImageContentData.

Behavior:
- scanImagesDirectory() in images.ts now scans both public and app directories for images.
- New helper functions getImageFolderPath() and processImageFile() in images.ts for image path resolution and processing.
- Updates image handling in listenForAssetMessages() in asset.ts to use ImageContentData.
Components:
- Favicon.tsx and index.tsx in Site modals updated to handle favicon uploads and deletions using ImageContentData.
- ImageManager in image/index.ts refactored to use ImageContentData for image operations.
- ImagesTab in ImageTab/index.tsx updated for image upload, delete, and rename operations using ImageContentData.
Models:
- ImageContentData in action.ts now includes an optional folder field.

^{This description was created by}^{for 8684523. You can customize this summary. It will automatically update as commits are pushed.}

ellipsis-dev

❌ Changes requested. Reviewed everything up to 576e202 in 2 minutes and 19 seconds

More details

Looked at 536 lines of code in 7 files
Skipped 0 files when reviewing.
Skipped posting 7 drafted comments based on config settings.

1. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:115

Draft comment:
The file input is now limited to '.ico' files. Confirm this restriction is intended for favicon uploads only.
Reason this comment was not posted:
Confidence changes required: 30% <= threshold 50%
None

2. apps/studio/src/components/Modals/Settings/Site/index.tsx:98

Draft comment:
When handling uploaded images (non-favicon), the code calls editorEngine.image.upload(uploadedImage) but then manually constructs an image path using uploadedImage.name. Ensure that this approach aligns with the new unique-naming strategy in the image manager.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. apps/studio/src/lib/editor/engine/image/index.ts:93

Draft comment:
Using setTimeout to trigger image scanning after upload/rename might be fragile. Consider handling scanning after confirming the file operation completes.
Reason this comment was not posted:
Confidence changes required: 40% <= threshold 50%
None

4. apps/studio/src/routes/editor/LayersPanel/ImageTab/index.tsx:420

Draft comment:
When opening the image folder, the code concatenates imageFolder with image.folder. Confirm that image.folder is always defined to prevent producing an invalid path.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

5. apps/studio/electron/main/assets/images.ts:25

Draft comment:
Using synchronous readFileSync in processImageFile might block the event loop for large images. Consider using fs.promises.readFile for asynchronous reading.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

6. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:115

Draft comment:
The accept attribute was changed from 'image/*' to '.ico'. If this is intentional for favicon support, consider adding a comment for clarification.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

7. Overall PR Description:1

Draft comment:
The PR title and description mention onboarding documentation changes, but the diff only includes refactoring for the images manager. Please clarify if documentation updates are expected.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_zn8gIwmcOt6tRrrv

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-04-15T11:21:17Z

apps/studio/electron/main/assets/images.ts

+
+// Helper function to validate and process image file
+function processImageFile(filePath: string, folder: string): ImageContentData {
+    const image = readFileSync(filePath, { encoding: 'base64' });


Consider using an async file read (fs.readFile) in processImageFile instead of readFileSync to avoid blocking the event loop.

ellipsis-dev · 2025-04-15T11:21:17Z

apps/studio/electron/main/assets/images.ts

        const imagePath = path.join(imageFolder, uniqueFileName);

-        const buffer = Buffer.from(content, 'base64');
+        const buffer = Buffer.from(image.content, 'base64');


Buffer.from(image.content, 'base64') in saveImageToProject may fail if image.content includes a data URI prefix (e.g. 'data:image/png;base64,'). Consider stripping the prefix before decoding.

Suggested change

const buffer = Buffer.from(image.content, 'base64');

const buffer = Buffer.from(image.content.replace(/^data:[^,]+,/, ''), 'base64');

ellipsis-dev · 2025-04-15T11:21:17Z

apps/studio/src/routes/editor/LayersPanel/ImageTab/index.tsx

@@ -418,7 +418,7 @@ const ImagesTab = observer(() => {
                                                        }
                                                        invokeMainChannel(
                                                            MainChannels.OPEN_IN_EXPLORER,
-                                                            imageFolder,
+                                                            `${imageFolder}${image.folder}`,


When concatenating imageFolder and image.folder, verify that image.folder is defined. Otherwise, the resulting path might include 'undefined'.

Suggested change

`${imageFolder}${image.folder}`,

`${imageFolder}${image.folder ? image.folder : ''}`,

spartan-uyen · 2025-04-16T01:47:17Z

[SKIP]
Title: Deleted favicon image still persists on the live site

Summary: When setting a favicon and later deleting that image from the Images section, the favicon remains visible on the live site, even though the site settings indicate the default favicon is restored.

Steps to Reproduce:

Upload a custom image and set it as the favicon.
Delete that image from the Images section.
Go to Site Settings and confirm that the favicon has reverted to the default one.
Visit or refresh the live site.

Actual Result:
The live site still displays the deleted favicon image.
Uploading screen-recording-2025-04-16-at-084017-1_cyV3oSHf.mp4…

Expected Result:
The live site should fall back to the default favicon immediately after the custom favicon is deleted.

ellipsis-dev

❌ Changes requested. Reviewed everything up to daf65d5 in 2 minutes and 44 seconds

More details

Looked at 542 lines of code in 7 files
Skipped 0 files when reviewing.
Skipped posting 5 drafted comments based on config settings.

1. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:115

Draft comment:
Ensure restricting the file input to only ".ico" files is intentional, as drag-and-drop may allow other image types.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
The comment is asking the author to confirm their intention regarding restricting file input to .ico files. This violates the rule against asking the PR author to confirm their intention. However, it does point out a potential issue with drag-and-drop allowing other image types, which could be useful information for the author to consider. The comment could be rephrased to suggest a specific action or improvement instead of asking for confirmation.

2. apps/studio/src/routes/editor/LayersPanel/ImageTab/index.tsx:416

Draft comment:
Consider using a proper path join rather than string concatenation when opening the folder to ensure correct paths across platforms.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The code already imports and uses platformSlash for path handling. The current concatenation with template literals is using the correct separator. While path.join() might be slightly more robust, the current code is already handling platform-specific path separators correctly. The suggested change is a minor optimization at best.
The comment raises a valid point about robust path handling. Path.join() could handle edge cases better like double slashes or missing slashes.
However, the code is already using a platform-specific separator and the current implementation is sufficient for this simple path concatenation. The change would be an over-optimization.
The comment should be deleted as the current code is already handling paths appropriately with platformSlash and the suggested change would not meaningfully improve the code.

3. apps/studio/src/lib/editor/engine/image/index.ts:173

Draft comment:
Remove debug logging before production. 'console.log(this.images)' in scanImages() may leak internal state.
Reason this comment was not posted:
Marked as duplicate.

4. apps/studio/src/lib/editor/engine/image/index.ts:113

Draft comment:
Using nanoid(4) for fileName generation might risk collisions. Consider using a longer id to ensure uniqueness.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. apps/studio/src/routes/editor/LayersPanel/ImageTab/index.tsx:416

Draft comment:
Consider using a path-joining utility (e.g. path.join) instead of string concatenation when constructing folder paths to ensure proper handling of path separators across platforms.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%

The code already uses platformSlash for path handling. 2. The current implementation with template literals is actually using a controlled path separator. 3. The code base seems to have an established pattern for path handling. 4. The suggestion would change the established pattern without clear benefit.
The comment might have a point about cross-platform compatibility, and path.join is generally a good practice. The existing platformSlash solution might have edge cases I'm not considering.
The code already has a platform-aware solution with platformSlash, and changing to path.join would be inconsistent with the codebase's established pattern.
Delete the comment since the code already handles platform-specific path separators appropriately using platformSlash.

Workflow ID: wflow_CaKxr4tZYRNPHmHY

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-04-16T03:14:45Z

apps/studio/electron/main/assets/images.ts

+
+// Helper function to validate and process image file
+function processImageFile(filePath: string, folder: string): ImageContentData {
+    const image = readFileSync(filePath, { encoding: 'base64' });


Consider using asynchronous file reading (fs.readFile) in processImageFile to avoid blocking the main thread.

ellipsis-dev · 2025-04-16T03:14:45Z

apps/studio/src/lib/editor/engine/image/index.ts

@@ -166,6 +170,7 @@ export class ImageManager {
        } else {
            this.images = [];
        }
+        console.log(this.images);


Remove or disable debug console.log statements in production code.

Suggested change

console.log(this.images);

apps/studio/electron/main/assets/images.ts

ellipsis-dev

❌ Changes requested. Incremental review on bdfa33c in 1 minute and 44 seconds

More details

Looked at 25 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 3 drafted comments based on config settings.

1. apps/studio/src/lib/editor/engine/image/index.ts:170

Draft comment:
Removed redundant console.log statement; ensure logging is handled elsewhere if needed.
Reason this comment was not posted:
Confidence changes required: 10% <= threshold 50%
None

2. apps/studio/electron/main/assets/images.ts:132

Draft comment:
Good use of regex to strip data URI prefix. Consider explicitly ensuring that image.content is defined before invoking replace, so Buffer.from doesn’t receive undefined. For instance, validate or provide a fallback.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

3. apps/studio/src/lib/editor/engine/image/index.ts:170

Draft comment:
Removed debug console.log – a good cleanup.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

Workflow ID: wflow_sUl7DK0HVwEqpAj9

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-04-16T07:10:10Z

apps/studio/electron/main/assets/images.ts

        const imagePath = path.join(imageFolder, uniqueFileName);

-        const buffer = Buffer.from(content, 'base64');
+        const buffer = Buffer.from(image.content?.replace(/^data:[^,]+,/, ''), 'base64');


Optional chaining on image.content may hide errors if content is undefined. Confirm if image.content is guaranteed.

Suggested change

const buffer = Buffer.from(image.content?.replace(/^data:[^,]+,/, ''), 'base64');

const buffer = Buffer.from(image.content.replace(/^data:[^,]+,/, ''), 'base64');

ellipsis-dev

👍 Looks good to me! Incremental review on 2e8c668 in 1 minute and 12 seconds

More details

Looked at 29 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 5 drafted comments based on config settings.

1. apps/studio/electron/main/assets/images.ts:132

Draft comment:
Good check on empty content and stripping the data URL prefix before conversion. Consider verifying whether the content always follows a data URL format as expected.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

2. apps/studio/src/lib/editor/engine/image/index.ts:12

Draft comment:
Removal of the console log is appropriate for production. Ensure that any needed logging is routed through a proper logging mechanism.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

3. apps/studio/electron/main/assets/images.ts:132

Draft comment:
Good defensive check for empty image content; ensure the input is a valid data URL or raw base64 string.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

4. apps/studio/src/lib/editor/engine/image/index.ts:170

Draft comment:
Removed unnecessary debug logging; this cleanup improves production readiness.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

5. apps/studio/electron/main/assets/images.ts:132

Draft comment:
Consider changing the error message from 'Can not save image with empty content' to 'Cannot save image with empty content' for better readability and consistency.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While "Cannot" is technically more formal than "Can not", this is an extremely minor stylistic change that doesn't impact code quality or functionality. The error message is already clear and understandable. This kind of nitpicking creates noise in code reviews and doesn't provide meaningful value.
Maybe maintaining consistent error message style across the codebase is important for professionalism and readability?
While consistency is good, this single instance is too minor to warrant a comment. The message is already clear and understandable.
Delete this comment as it's an extremely minor stylistic suggestion that doesn't meaningfully improve the code.

Workflow ID: wflow_5TXs3dDwpTQdvQNJ

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 8684523 in 32 seconds. Click for details.

Reviewed 41 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 6 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. apps/studio/electron/main/assets/images.ts:24

Draft comment:
Good conversion to async. Ensure all callers handle promises correctly.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

2. apps/studio/electron/main/assets/images.ts:48

Draft comment:
Awaiting processImageFile in loop sequentially. Consider Promise.all for performance improvements if order isn’t required.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

3. apps/studio/electron/main/assets/images.ts:60

Draft comment:
Sequential await in loop; if performance becomes an issue, consider use of Promise.all.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

4. apps/studio/electron/main/assets/images.ts:24

Draft comment:
Good use of async fs.readFile in processImageFile – ensures non-blocking I/O.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

5. apps/studio/electron/main/assets/images.ts:48

Draft comment:
Consider using Promise.all for concurrent image processing in loops to boost performance.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

6. apps/studio/electron/main/assets/images.ts:132

Draft comment:
Minor typographical issue: Consider replacing 'Can not save image with empty content' with 'Cannot save image with empty content' for improved clarity.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_HmKmpPdCkDRobR49

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

* refactor: update images manager * add checking null

* 'main' of https://github.com/onlook-dev/onlook: use DomElementStyles (onlook-dev#1810) feat: Added file watcher & auto update modified opened files in Dev Tab (onlook-dev#1802) fix: border thickness hidden when setting border color from color picker (onlook-dev#1805) move overflow input (onlook-dev#1800) Added overflow option in the style panel (onlook-dev#1793) refactor: update images manager (onlook-dev#1784) chore: error nesting button (onlook-dev#1790) refactor: update naming for brand color (onlook-dev#1785) Tys toolbar v4 (onlook-dev#1786)

* refactor: update images manager * add checking null

ellipsis-dev bot reviewed Apr 15, 2025

View reviewed changes

spartan-vutrannguyen marked this pull request as draft April 15, 2025 11:22

spartan-vutrannguyen added 2 commits April 16, 2025 08:52

refactor: update images manager

3fa75c0

add checking null

daf65d5

spartan-vutrannguyen force-pushed the refactor/update-images-manager branch from 9515dae to daf65d5 Compare April 16, 2025 01:52

spartan-vutrannguyen marked this pull request as ready for review April 16, 2025 03:11

spartan-vutrannguyen requested a review from Kitenite April 16, 2025 03:12

ellipsis-dev bot reviewed Apr 16, 2025

View reviewed changes

clean up

2e8c668

spartan-vutrannguyen force-pushed the refactor/update-images-manager branch from bdfa33c to 2e8c668 Compare April 16, 2025 07:13

ellipsis-dev bot reviewed Apr 16, 2025

View reviewed changes

use none-blocking writefile

8684523

ellipsis-dev bot reviewed Apr 20, 2025

View reviewed changes

Kitenite merged commit cd52dfa into main Apr 20, 2025

Kitenite deleted the refactor/update-images-manager branch April 20, 2025 01:23

ml-delaurier pushed a commit to ml-delaurier/nolook that referenced this pull request Apr 23, 2025

refactor: update images manager (onlook-dev#1784)

9fc019a

* refactor: update images manager * add checking null

t1c1 pushed a commit to t1c1/onlookbotcodes that referenced this pull request Jun 5, 2025

refactor: update images manager (onlook-dev#1784)

4dbb535

* refactor: update images manager * add checking null

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: update images manager #1784

refactor: update images manager #1784

Uh oh!

spartan-vutrannguyen commented Apr 15, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot Apr 15, 2025

Uh oh!

ellipsis-dev bot Apr 15, 2025

Uh oh!

ellipsis-dev bot Apr 15, 2025

Uh oh!

spartan-uyen commented Apr 16, 2025 •

edited

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot Apr 16, 2025

Uh oh!

ellipsis-dev bot Apr 16, 2025

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot Apr 16, 2025

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

Uh oh!

	const buffer = Buffer.from(image.content, 'base64');
	const buffer = Buffer.from(image.content.replace(/^data:[^,]+,/, ''), 'base64');

	`${imageFolder}${image.folder}`,
	`${imageFolder}${image.folder ? image.folder : ''}`,

refactor: update images manager #1784

refactor: update images manager #1784

Uh oh!

Conversation

spartan-vutrannguyen commented Apr 15, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Type of Change

Testing

Screenshots (if applicable)

Additional Notes

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

spartan-uyen commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spartan-vutrannguyen commented Apr 15, 2025 •

edited by ellipsis-dev bot

Loading

spartan-uyen commented Apr 16, 2025 •

edited

Loading