feat: update metadata of page #1766
Merged
Conversation
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to fc5e110 in 4 minutes and 2 seconds
More details
- Looked at
1700lines of code in17files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. apps/studio/electron/main/pages/update.ts:255
- Draft comment:
Remove or disable debugging log before committing. - 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.
2. apps/studio/src/lib/editor/engine/pages/helper.ts:43
- Draft comment:
Remove debugging logs from normalize/compare functions. - Reason this comment was not posted:
Marked as duplicate.
3. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:67
- Draft comment:
Provide an appropriate alt text for thetag for accessibility.
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
This is a legitimate accessibility concern - img tags should have alt text. However, in this specific case, the image is purely decorative as it's showing a preview of the favicon that was just uploaded in the same component. The same image is already being shown as a background image. The img tag appears redundant and might even be causing the image to show twice.
Maybe the img tag serves some purpose I'm not seeing, like handling different image formats or sizes that the background-image property doesn't handle well.
Even if the img tag is needed, since it's showing the same content as the background image and is purely decorative, an empty alt="" would be more appropriate than adding descriptive alt text.
While accessibility is important, this specific comment suggesting descriptive alt text isn't quite right for this use case. The code likely needs a different fix - either removing the redundant img tag or using alt="".
4. apps/studio/src/lib/editor/engine/pages/index.ts:255
- Draft comment:
Remove commented-out code and debugging logs to clean up the code. - Reason this comment was not posted:
Marked as duplicate.
5. Overall:1
- Draft comment:
Ensure PR description matches changes: metadata update and site settings improvements are implemented, but onboarding docs updates are not present. - 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.
6. apps/studio/electron/main/events/page.ts:88
- Draft comment:
Consider providing explicit error logging or throwing an error when routerConfig.type isn’t 'app' instead of silently returning. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The silent return could hide bugs, making debugging harder. However, this might be intentional behavior - perhaps non-app router types are expected and should be silently ignored. Without knowing the broader system design and requirements around router type handling, we can't be certain that throwing an error is the right approach. The comment is speculative without strong evidence that this is definitely wrong.
I might be too cautious - silent failures are generally considered bad practice. Also, if non-app routers should be handled, they should probably be handled explicitly.
While silent failures can be problematic, without clear evidence that this is incorrect behavior rather than intentional, suggesting error throwing could introduce incorrect behavior. The current implementation might be deliberately ignoring non-app routers.
Delete the comment since we don't have strong evidence that the current behavior is incorrect, and suggesting error throwing without understanding the intended behavior could be harmful.
7. apps/studio/electron/main/pages/scan.ts:128
- Draft comment:
Remove the debugging console.log(metadata) statement before committing. - Reason this comment was not posted:
Marked as duplicate.
8. apps/studio/src/lib/editor/engine/pages/helper.ts:43
- Draft comment:
Remove or disable debugging console.log statements to clean up production logs. - Reason this comment was not posted:
Marked as duplicate.
9. apps/studio/src/lib/editor/engine/pages/index.ts:255
- Draft comment:
Remove the debugging console.log statement (logging projectRoot, pagePath, metadata, isRoot) before invoking the updateChannel. - Reason this comment was not posted:
Marked as duplicate.
10. apps/studio/src/lib/editor/engine/index.ts:47
- Draft comment:
Document that extending _settingsTab to allow generic strings (SettingsTabValue | string) is intentional. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to document their intention behind a change. This falls under the rule of not asking the author to confirm their intention or to update documentation. Therefore, this comment should be removed.
11. apps/studio/src/components/Modals/Settings/Site/Page.tsx:12
- Draft comment:
Typographical error in DEFAULT_DESCRIPTION: 'yourpage title' should be 'your page title'. - Reason this comment was not posted:
Marked as duplicate.
12. apps/studio/src/components/Modals/Settings/index.tsx:9
- Draft comment:
The import on Line 9 uses './Advance' as the file path for AdvancedTab. Please verify if the file is actually named './Advanced' and update accordingly. - 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.
13. apps/studio/src/lib/editor/engine/pages/index.ts:252
- Draft comment:
Typo in the commented error message: 'A page with this name does not exists' should be 'A page with this name does not exist'. - 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 in question is commented out, meaning it's not actually being used. The grammatical error, while real, is in inactive code. Making comments about inactive code is not productive since the code may never be uncommented or could be removed entirely. Additionally, this is a very minor grammatical issue that doesn't affect functionality.
The grammar error could indicate a pattern of poor attention to detail that might be worth addressing. Also, if the code gets uncommented later, the error would persist.
While those points have merit, commenting on inactive code violates our principle of only making comments that require clear, actionable changes. The code may never be used, making this potentially wasted effort.
Delete this comment as it addresses a minor grammatical issue in commented-out code that may never be used.
14. packages/models/src/constants/ipc.ts:64
- Draft comment:
Typographical inconsistency: The constant GET_TEMPLATE_NODE_CLASS is defined with a singular form in its identifier, but its assigned string value is 'get-template-node-classes' (plural). Consider aligning the identifier and the value (either both singular or both plural) for consistency. - 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_h3N4Tn2tKW5GD5SC
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.
|
|
||
| // Extract metadata from the page file | ||
| const metadata = await extractMetadata(path.join(dir, pageFile.name)); | ||
| console.log(metadata); |
There was a problem hiding this comment.
Remove debug console.log before production.
Suggested change
| console.log(metadata); |
|
|
||
| const DEFAULT_TITLE = 'Saved Places - Discover new spots'; | ||
| const DEFAULT_DESCRIPTION = | ||
| 'This is the information that will show up on search engines below yourpage title.'; |
There was a problem hiding this comment.
Fix the typo in the default description – 'yourpage title' should be 'your page title'.
Suggested change
| 'This is the information that will show up on search engines below yourpage title.'; | |
| 'This is the information that will show up on search engines below your page title.'; |
|
[SOLVED] Summary: Attempting to update the site title or description results in an error, which is shown directly on the screen. Steps to Reproduce:
Actual Result: Expected Result: |
|
[SOLVED] Summary: When saving changes to title, description, or image in site settings, there is no success/failure message, and the settings popup does not close. Steps to Reproduce:
Actual Result:
Expected Result:
|
|
Title: Cannot duplicate a page that has already been duplicated Summary: When attempting to duplicate a page that was previously duplicated (e.g., test001-copy), the system throws an error instead of creating a new duplicate. Steps to Reproduce:
Actual Result: Expected Result: |
|
[SOLVED] Summary: After editing the title/description/image of a page and saving changes, switching to a different page still shows the metadata of the previously edited page. Steps to Reproduce:
Actual Result: Expected Result: |
|
[SOLVED] Summary: When only the image is modified in the metadata editor without changing the title or description, the Save Changes button does not work and the changes are not saved. Steps to Reproduce:
Actual Result:
Expected Result:
|
@spartan-uyen Please create a new issue for this since this is an error with page |
I added the popup to show success or error. But I keep the modal setting open in case user want to edit other pages |
@spartan-uyen Please help me check again, I can not reproduce this. Please remember to Save changes before switching pages |
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to fb49350 in 2 minutes and 54 seconds
More details
- Looked at
1779lines of code in23files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:82
- Draft comment:
Consider adding an alt attribute to thetag for accessibility.
- 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%
While accessibility is important, this is a special case. The img is showing a favicon preview inside a container that already has the image as a background. The img tag appears to be redundant with the background-image style. The alt text wouldn't provide additional value since this is just a preview of what was uploaded.
I could be wrong about the purpose of having both background-image and img tag - maybe they serve different purposes. Also, accessibility best practices generally do recommend alt tags.
Even if both image displays are needed, the alt text wouldn't provide meaningful value here since this is just showing a preview of what the user just uploaded. The visual context is clear from the UI.
The comment should be removed as adding alt text here wouldn't provide meaningful accessibility value given the context of this being a favicon preview.
2. apps/studio/electron/main/pages/update.ts:242
- Draft comment:
Consider handling unexpected file formats or non-object metadata values when constructing the Babel AST. Although the current implementation covers strings, objects, and arrays, ensure future metadata types are supported. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/studio/src/lib/editor/engine/pages/index.ts:245
- Draft comment:
Ensure that error messages rethrown after invoking the main channel are user-friendly and consider logging additional context for debugging purposes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/studio/electron/main/events/code.ts:198
- Draft comment:
Verify if using fontFileWatcher.watch for SCAN_SITE_CONFIG is intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to verify their intention, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.
5. apps/studio/src/routes/editor/LayersPanel/ImageTab/index.tsx:446
- Draft comment:
Confirm if the open condition for RenameImageModal is correct; it only opens when newImageName differs from imageToRename. - 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.
6. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:14
- Draft comment:
Consider adding 'editorEngine.image.assets' to the useEffect dependency array to update selectedImage when assets change. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The effect does use editorEngine.image.assets inside, so technically it should be in the dependency array according to React's rules. However, this seems to be a one-time setup effect that runs when the url prop changes to find the matching image in assets. It's not clear if we actually want to re-run this when assets change - that could cause unwanted image updates.
I could be wrong about the component's intended behavior - maybe it should update the selected image whenever the assets list changes. Without more context about the broader application behavior, I can't be 100% certain.
While the missing dependency is technically correct, suggesting changes to effect dependencies without understanding the full intended behavior could cause bugs. The current code may be intentionally written this way.
Given the uncertainty about intended behavior and potential for introducing bugs, this comment should be removed despite being technically correct about React's dependency rules.
7. apps/studio/src/routes/editor/TopBar/ProjectSelect/index.tsx:179
- Draft comment:
Ensure that scanning project metadata on settings click is intended. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. apps/studio/src/lib/editor/engine/pages/index.ts:252
- Draft comment:
Typo in the commented out error message: "A page with this name does not exists" should be "A page with this name does not exist". - 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 in question is commented out, meaning it's not actually being used. The comment is technically correct about the grammar, but since the code is inactive and may be deleted entirely, pointing out grammar in commented code seems unnecessary. Additionally, the logic of the error message itself seems incorrect - the condition checks if the route exists but the message says it doesn't exist.
The grammar fix would be correct if the code was active. The commented code may be intended to be uncommented later.
Since the code is commented out and may be deleted or significantly changed before being used, fixing grammar in inactive code provides no real value. The logic issue suggests this code needs more substantial review if it's ever activated.
Delete this comment since it addresses grammar in commented-out code that isn't currently used and may need broader revision.
9. apps/studio/src/routes/editor/TopBar/ProjectSelect/index.tsx:126
- Draft comment:
Minor typographical issue: In the className string on line 126, the classes 'row' and 'center' are likely incorrect if the intent was to align items using Tailwind CSS. Consider using 'flex flex-row items-center' instead to be consistent with the rest of the code. - 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.
10. packages/models/src/constants/ipc.ts:54
- Draft comment:
Typo detected: The constant CLEAN_CODE_KEYS is assigned the value 'clean-move-keys'. This appears inconsistent. Perhaps it should be 'clean-code-keys' to match the key name? - 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.
11. packages/models/src/constants/ipc.ts:64
- Draft comment:
Typo alert: The constant GET_TEMPLATE_NODE_CLASS is assigned the value 'get-template-node-classes'. The key is singular 'CLASS' while its value is plural ('classes'). Consider aligning these names to avoid confusion. - 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_ez8HCza1FtgwQBBH
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.
| @@ -40,9 +40,13 @@ export const validateNextJsRoute = (route: string): { valid: boolean; error?: st | |||
|
|
|||
| export const doesRouteExist = (nodes: PageNode[], route: string): boolean => { | |||
| const normalizedRoute = normalizeRoute(route); | |||
| console.log(nodes, route); | |||
There was a problem hiding this comment.
Remove debug console.log statements before production.
Suggested change
| console.log(nodes, route); |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 28639ad in 49 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/studio/src/components/Modals/Settings/Site/index.tsx:52
- Draft comment:
Validate URL construction before using new URL(). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. apps/studio/src/components/Modals/Settings/Site/index.tsx:9
- Draft comment:
Ensure new default title and description align with overall app branding. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/studio/src/components/Modals/Settings/Site/index.tsx:9
- Draft comment:
Confirm that the new default title and description ('My New App' / 'Generated by Onlook') are intentional and align with current branding/design. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/src/components/Modals/Settings/Site/index.tsx:52
- Draft comment:
Good improvement handling metadataBase. Consider abstracting the URL normalization (prefixing with 'https://') into a helper to reduce repetition. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/src/components/Modals/Settings/Site/index.tsx:52
- Draft comment:
Consider handling the case where neither project.domains?.base?.url nor project.url is provided to avoid a scenario with an undefined metadataBase. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/src/components/Modals/Settings/Site/index.tsx:1
- Draft comment:
PR description/title inconsistency: The PR title suggests a metadata update, but the issue details mention onboarding docs. Please ensure the description aligns with the code changes. - 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_Pv49vUeMfA63TKx9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
spartan-vutrannguyen
changed the title
|
[SOLVED] Summary: After updating the metadata (title/description) and clicking Save Changes, switching to a different page and then returning causes the data to revert to the previous state, indicating that the changes were not persisted. Steps to Reproduce:
Actual Result:
Expected Result: |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 58d7e4f in 1 minute and 45 seconds
More details
- Looked at
127lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/studio/src/components/Modals/Settings/Site/MetadataForm.tsx:23
- Draft comment:
Remove or update unused defaultTitle/defaultDescription props in the interface, as they are no longer used in the component. - 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.
2. apps/studio/src/components/Modals/Settings/Site/MetadataForm.tsx:43
- Draft comment:
Remove unused defaultTitle/defaultDescription from MetadataFormProps; the component now relies on internal constants. - 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/components/Modals/Settings/Site/index.tsx:9
- Draft comment:
PR description mentions adding framework documentation for onboarding, but this diff only updates metadata. Confirm if documentation changes are intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm their intention regarding the documentation changes. It violates the rule against asking the author to confirm their intention or to update the PR description. Therefore, it should be removed.
4. apps/studio/src/lib/editor/engine/pages/index.ts:251
- Draft comment:
Typo in error message: consider using 'does not exist' instead of 'does not exists'. - Reason this comment was not posted:
Marked as duplicate.
5. apps/studio/src/lib/editor/engine/pages/index.ts:252
- Draft comment:
Typo in error message: 'A page with this name does not exists' should be 'A page with this name does not exist'. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_67xYLefbi39vt0sB
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.
|
Title: Discard does not revert metadata image Summary: When clicking "Discard" after editing metadata, the title and description are correctly reverted, but the updated image remains and is not reset. Steps to Reproduce:
Actual Result:
Expected Result: |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9d0dd14 in 1 minute and 22 seconds
More details
- Looked at
279lines of code in3files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:26
- Draft comment:
Consider including 'editorEngine.image.assets' in the dependency array of this useEffect to ensure it reacts to asset changes. - 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.
2. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:95
- Draft comment:
Remember to consider revoking the object URL to prevent memory leaks. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is purely informative, reminding the author to consider revoking the object URL to prevent memory leaks. It doesn't suggest a specific code change or ask for a test to be written. It violates the rule against making purely informative comments.
3. apps/studio/src/components/Modals/Settings/Site/Image.tsx:26
- Draft comment:
Include 'editorEngine.image.assets' in the dependency array of this useEffect to reflect asset updates. - 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.
4. apps/studio/src/components/Modals/Settings/Site/Image.tsx:94
- Draft comment:
Consider revoking the object URL created in saveImage to avoid memory leaks. - Reason this comment was not posted:
Comment was on unchanged code.
5. apps/studio/src/components/Modals/Settings/Site/MetadataForm.tsx:146
- Draft comment:
Ensure that onFaviconSelect is always defined when showFavicon is true to avoid runtime errors. - Reason this comment was not posted:
Comment was on unchanged code.
6. apps/studio/src/components/Modals/Settings/Site/MetadataForm.tsx:1
- Draft comment:
The PR description mentions updates for onboarding docs; ensure that documentation updates are included if required by the issue. - 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.
7. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:45
- Draft comment:
Consider reordering the code so that 'saveImage' is defined before its use in the drag handlers to avoid potential temporal dead zone issues. - 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.
8. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:26
- Draft comment:
In the useEffect, if no matching image asset is found, consider setting selectedImage to the provided url so the state remains consistent. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:123
- Draft comment:
Add an alt attribute to thetag to improve accessibility.
- 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.
10. apps/studio/src/components/Modals/Settings/Site/Image.tsx:26
- Draft comment:
Consider adding an else branch in the useEffect to set selectedImage to url when no matching asset is found. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
11. apps/studio/src/components/Modals/Settings/Site/Image.tsx:45
- Draft comment:
Reorder the declaration of 'saveImage' so it comes before its usage in drag event handlers. - 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.
12. apps/studio/src/components/Modals/Settings/Site/Image.tsx:132
- Draft comment:
The UploadButton component is duplicated in both Image and Favicon files; consider extracting it into a shared component to reduce repetition. - Reason this comment was not posted:
Comment was on unchanged code.
13. apps/studio/src/components/Modals/Settings/Site/MetadataForm.tsx:147
- Draft comment:
Avoid using a non-null assertion for onFaviconSelect. Ensure it's provided when showFavicon is true or add appropriate null checks. - Reason this comment was not posted:
Comment was on unchanged code.
14. apps/studio/src/components/Modals/Settings/Site/MetadataForm.tsx:1
- Draft comment:
The PR description mentions updates to onboarding docs and framework documentation, but the changes here focus on metadata handling. Ensure that the documentation updates are also included if intended. - 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_NCjLnItEDGmSh15z
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Kitenite
reviewed
Apr 13, 2025
| console.error(`Failed to get mime type for ${imagePath}`); | ||
| continue; | ||
| } | ||
| const mimeType = mime.getType(imagePath) || 'image/x-icon'; |
There was a problem hiding this comment.
I don't think we want to default for icon when mime type is not found. What's the reason for this change instead of explicitly handling icons?
Kitenite
reviewed
Apr 13, 2025
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on c2ecc74 in 1 minute and 28 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. apps/web/template/.gitignore:15
- Draft comment:
Duplicate entry for 'next-env.d.ts' found. Consider removing one. - Reason this comment was not posted:
Marked as duplicate.
2. apps/web/template/.gitignore:37
- Draft comment:
There is a duplicate entry for 'next-env.d.ts' (appearing on line 15 and line 37). While this doesn't affect functionality, it can be removed to avoid redundancy and maintain 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_xq8ZDFxo6vtBJb6C
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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b3ce848 in 1 minute and 19 seconds
More details
- Looked at
429lines of code in14files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. apps/studio/electron/main/events/code.ts:195
- Draft comment:
Removal of duplicate SCAN_SITE_CONFIG handler is intentional? Ensure that the behavior change is expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that the behavior change is expected, which violates the rule against asking for confirmation of intention or ensuring behavior. It does not provide a specific suggestion or point out a clear issue.
2. apps/studio/electron/main/events/page.ts:76
- Draft comment:
Updated parameter type to PageMetadata. Confirm that all consumers have been updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm that all consumers have been updated, which violates the rule against asking the author to confirm or ensure things. It does not provide a specific code suggestion or point out a specific issue.
3. apps/studio/electron/main/pages/update.ts:213
- Draft comment:
Extra trailing comma in function parameters? Verify if this is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
Trailing commas in parameter lists are a widely accepted practice in modern JavaScript/TypeScript. They make git diffs cleaner when adding new parameters and are considered good style. The comment is questioning something that is actually good practice.
Maybe there's a specific style guide for this project that prohibits trailing commas?
Even if there was a style guide rule against trailing commas, this would be better handled by automated linting rather than code review comments.
This comment should be deleted as it questions a valid and commonly accepted coding practice that, if it was an issue, would be better handled by linting.
4. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:123
- Draft comment:
Good conditional rendering ofelement. Ensure that alt attribute is added for accessibility.
- Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
5. apps/studio/src/components/Modals/Settings/Site/MetadataForm.tsx:1
- Draft comment:
Imports are reordered based on dependencies. Confirm that the new order doesn't break tree shaking. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
6. apps/studio/src/components/Modals/Settings/Site/Page.tsx:32
- Draft comment:
Metadata update in handleSave correctly uses PageMetadata. Double-check that openGraph merging behavior is as intended. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
7. apps/studio/src/hooks/useMetadataForm.ts:15
- Draft comment:
State initialization uses initialMetadata with fallback defaults; ensure updates flow correctly when initialMetadata changes. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
8. packages/models/src/pages/index.ts:9
- Draft comment:
Updated metadata field in PageNode interface; validate that all dependent modules reflect this type change. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to validate that all dependent modules reflect the type change, which is against the rules as it is asking for confirmation and validation. It does not provide a specific code suggestion or point out a specific issue.
9. apps/studio/electron/main/pages/update.ts:59
- Draft comment:
Ensure the 'Metadata' import from 'next' (added if missing) is consistent with the updated PageMetadata type. Consider aliasing or verifying compatibility to avoid type mismatches. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
10. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:123
- Draft comment:
Add an appropriate alt attribute to thetag for improved accessibility.
- Reason this comment was not posted:
Comment was on unchanged code.
11. apps/studio/src/lib/editor/engine/pages/index.ts:47
- Draft comment:
There's a duplicate check for folder nodes (children) in isNodeActive. Remove one redundant check to simplify the logic. - 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.
12. apps/web/template/.gitignore:15
- Draft comment:
Removal of 'next-env.d.ts' from .gitignore may be unintended. Verify that this file should be committed as per Next.js recommendations. - 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.
13. apps/studio/electron/main/pages/scan.ts:40
- Draft comment:
Consider extending metadata extraction to handle non-string literal types (e.g., numbers or booleans) for robustness. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
Workflow ID: wflow_zvXMRDzte51ur65u
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Kitenite
approved these changes
Apr 13, 2025
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a312021 in 1 minute and 12 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. apps/studio/electron/main/assets/images.ts:23
- Draft comment:
Removing the fallback could produce an invalid data URI. If mime.getType returns null, the image URI becomes 'data:null;base64,...'. Please consider a fallback (e.g., 'image/x-icon') or a null-check. - 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 filters for specific image extensions (.jpg, .jpeg, .png, .gif, .webp, .svg, .ico) before processing. The mime-lite package should definitely recognize these standard image formats. The previous fallback to 'image/x-icon' was actually potentially misleading since it would mask any MIME type detection issues.
What if mime-lite has a bug or is misconfigured? What if there are file extension mismatches with actual content?
While those are edge cases, having null/undefined MIME types would actually be better than silently using wrong ones - it would make issues more obvious during testing.
The comment should be removed. The code is safer without the fallback since it will fail fast if there are MIME type detection issues rather than masking them.
Workflow ID: wflow_RdXVqJliPYqHSeOE
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.
| @@ -21,10 +21,6 @@ async function scanImagesDirectory(projectRoot: string): Promise<ImageContentDat | |||
| const imagePath = path.join(imagesPath, entry.name); | |||
| const image = readFileSync(imagePath, { encoding: 'base64' }); | |||
| const mimeType = mime.getType(imagePath); | |||
There was a problem hiding this comment.
Potential bug: Removing the fallback could yield a null or undefined mimeType, causing an invalid data URI.
Suggested change
| const mimeType = mime.getType(imagePath); | |
| const mimeType = mime.getType(imagePath) || 'application/octet-stream'; |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2c370f4 in 37 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. apps/studio/electron/main/assets/images.ts:23
- Draft comment:
Good use of default MIME type. Consider adding an inline comment clarifying that 'application/octet-stream' is used as a fallback if mime.getType fails. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/electron/main/assets/images.ts:23
- Draft comment:
Good update using a fallback MIME. Verify that 'application/octet-stream' is acceptable for image data; if images are expected, consider a more specific default if needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_tc1BrRgJNLL7g4o0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 284b18f in 1 minute and 3 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:136
- Draft comment:
Removed hover opacity/transition classes. Verify that this change is intentional for styling consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment asks the PR author to verify if the removal of hover opacity/transition classes is intentional. This violates the rule against asking the author to confirm their intention. The comment does not provide a specific suggestion or point out a clear issue.
2. apps/studio/src/components/Modals/Settings/Site/Favicon.tsx:137
- Draft comment:
The button’s styling was changed by removing the hover-based opacity and transition classes. Confirm that this removal and the addition of a fixed margin (mt-2) is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic. Assume that if the author changed the UI, they did it correctly." This is exactly that case - it's purely about CSS classes affecting visual appearance. The comment also asks for confirmation of intention, which is another rule violation.
Maybe these styling changes could affect functionality or accessibility in some way that isn't purely visual?
The changes only affect opacity and margin which are purely visual properties. Even if there were accessibility concerns, we should trust the author per the rules.
This comment should be deleted as it violates two rules: commenting on pure UI styling changes and asking for confirmation of intention.
Workflow ID: wflow_p67KG2Ye9BFBJL3T
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1d7f465 in 1 minute and 7 seconds
More details
- Looked at
94lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. apps/studio/src/components/Modals/Settings/Site/Page.tsx:46
- Draft comment:
When baseUrl is undefined,https://${baseUrl}becomes 'https://undefined'. Validate baseUrl exists before using it. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/studio/src/components/Modals/Settings/Site/index.tsx:48
- Draft comment:
Ensure baseUrl is truthy before constructing the URL. Otherwise,https://${baseUrl}may become 'https://undefined'. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. apps/studio/src/components/Modals/Settings/Site/Page.tsx:47
- Draft comment:
Ensure baseUrl is defined before constructing the URL. If baseUrl is undefined,https://${baseUrl}becomes 'https://undefined', which is unintended. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. apps/studio/src/components/Modals/Settings/Site/index.tsx:49
- Draft comment:
Add a check to verify baseUrl is defined before using it to build the metadataBase URL. Otherwise, it might yield an invalid URL (e.g. 'https://undefined'). - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_M4squwkndMT205Hv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* handle image * add scan project metadata * refactor type * update scan pages * refactor duplicated upload image * fetch assets * handle use client case * handle page URL * update default title and default description * remove log * revert files * Update scan on settings modal open * Clean up from template * Update gitignore * Fix build error and rename Metadata to PageMetadata * remove default mime type * add default mime type * display upload button in favicon * update base url
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* handle image * add scan project metadata * refactor type * update scan pages * refactor duplicated upload image * fetch assets * handle use client case * handle page URL * update default title and default description * remove log * revert files * Update scan on settings modal open * Clean up from template * Update gitignore * Fix build error and rename Metadata to PageMetadata * remove default mime type * add default mime type * display upload button in favicon * update base url
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Screen.Recording.2025-04-11.at.09.22.49.mp4
Screen.Recording.2025-04-11.at.09.28.49.mp4
Additional Notes
Important
Enhances metadata handling for pages and sites with new components, hooks, and IPC channels, supporting metadata extraction, updating, and display.
PageMetadatainterface inpages/index.tsto define metadata structure.extractMetadata()inpages/scan.tsto parse metadata from files.updateMetadataInFile()inpages/update.tsto update metadata in files.updateNextJsPage()inpages/update.tsto handle metadata updates for Next.js pages.UPDATE_PAGE_METADATAandSCAN_PROJECT_METADATAchannels inconstants/ipc.ts.FaviconandImagePickercomponents inSite/Favicon.tsxandSite/Image.tsxfor image selection.MetadataForminSite/MetadataForm.tsxfor metadata input.PageTabandSiteTabcomponents inSite/Page.tsxandSite/index.tsxfor page and site settings.useMetadataFormhook inuseMetadataForm.tsfor managing metadata form state.PagesManagerinpages/index.tsto support metadata operations.ProjectsManagerinprojects/index.tsto scan and update project metadata..gitignoreinweb/template/.gitignoreto includenext-env.d.ts.EditorEngineinengine/index.tsto handle settings tab changes.This description was created by
for 1d7f465. It will automatically update as commits are pushed.