-
Notifications
You must be signed in to change notification settings - Fork 516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add component tab and creation functionality #1643
base: main
Are you sure you want to change the base?
feat: add component tab and creation functionality #1643
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 06147e1 in 3 minutes and 57 seconds
More details
- Looked at
807
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
15
drafted comments based on config settings.
1. apps/studio/electron/main/code/components.ts:164
- Draft comment:
Consider refining the regex in duplicateComponent to match the component base name more strictly (e.g. with boundaries) to avoid unintended matches. - 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 current regex already has ^ and $ anchors which provide strict boundaries. The regex is used to match file names, and the baseName is derived from the componentName parameter. The current implementation seems sufficiently strict for its purpose - matching numbered variants of a component file. Adding more boundaries likely wouldn't prevent any real-world issues.
The comment could be right that there are edge cases where the current regex might match unintended filenames, though I can't think of a realistic scenario where this would happen.
The existing regex already has appropriate boundaries and the suggestion seems overly cautious without demonstrating a real problem to solve.
The comment should be removed as it suggests a change that doesn't solve any concrete problem and the current implementation is already sufficiently robust.
2. apps/studio/electron/main/code/components.ts:218
- Draft comment:
The renaming logic in renameComponent only renames nodes whose name exactly matches the old file name. Verify that this covers all export cases for component renaming. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. apps/studio/electron/main/code/components.ts:196
- Draft comment:
Error messages in duplicateComponent (and similarly in renameComponent/createNewComponent) say 'Failed duplicating component: ...' using 'page'. Update these messages for clarity and consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. apps/studio/src/routes/editor/LayersPanel/ComponentsTab.tsx:103
- Draft comment:
Before using editorEngine.elements.selected[0] in create component handler, add a check to ensure an element is selected to avoid potential runtime errors. - 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 is in an isCreateAction block, which suggests this action is only triggered when an element is selected. The || '' fallback suggests the code already handles null/undefined cases. Without seeing how isCreateAction is set or how the editor engine works, we can't be 100% certain if elements.selected could be empty when this code runs.
I don't have visibility into how isCreateAction gets set or the editor engine's behavior. There could be edge cases where selected is empty even when isCreateAction is true.
While there's some uncertainty, the presence of null handling (|| '') and the controlled flow through isCreateAction suggests this was likely considered in the design.
Without stronger evidence that empty selection is a real issue, and given the existing null handling, this comment appears overly cautious and should be removed.
5. apps/studio/src/routes/editor/LayersPanel/index.tsx:89
- Draft comment:
Resetting editorEngine.componentPanelTab to LeftTabValue.LAYERS on tab click may override intended toggle behavior. Verify if this reset is intended. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. apps/studio/src/lib/editor/engine/projectinfo/index.ts:40
- Draft comment:
Error messages in duplicateComponent/renameComponent catch blocks mention 'page' instead of 'component'. - Reason this comment was not posted:
Marked as duplicate.
7. apps/studio/src/routes/editor/LayersPanel/ComponentsTab.tsx:83
- Draft comment:
Error logs in component actions reference 'page' instead of 'component'. - Reason this comment was not posted:
Marked as duplicate.
8. apps/studio/electron/main/code/components.ts:271
- Draft comment:
Typo: Remove the extraneous comma after the 'files: string[]' parameter in the function declaration of checkIfComponentIsUsed. This ensures proper syntax. - 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 comment is suggesting to remove a comma that is actually valid TypeScript syntax. In TypeScript/JavaScript, trailing commas in parameter lists are allowed and often preferred as they produce cleaner git diffs. The comma here is not a typo or syntax error. The comment is incorrect in suggesting this is a problem that needs to be fixed.
Could there be a style guide that prohibits trailing commas in parameter lists? Could the comma cause issues in some environments?
Trailing commas are widely accepted in modern JavaScript/TypeScript and supported in all modern environments. Even if there was a style preference against them, this would be a trivial stylistic issue that should be handled by automated formatting tools, not code review comments.
The comment should be deleted because it incorrectly suggests removing valid syntax, and even if it was a style preference, it would be too minor to warrant a code review comment.
9. apps/studio/src/lib/editor/engine/projectinfo/index.ts:7
- Draft comment:
Typographical error: The constant value 'creare-new-component' appears to be misspelled. It should likely be 'create-new-component'. - Reason this comment was not posted:
Marked as duplicate.
10. apps/studio/src/lib/editor/engine/projectinfo/index.ts:54
- Draft comment:
Typographical error in error logging: In the renameComponent method, the catch block logs 'Failed to duplicate page:' which seems to be a copy-paste mistake. It should reflect renaming (e.g., 'Failed to rename component:'). - Reason this comment was not posted:
Marked as duplicate.
11. apps/studio/src/lib/editor/engine/projectinfo/index.ts:77
- Draft comment:
Typographical error in error logging: In the createComponent method, the catch block logs 'Failed to duplicate page:' which appears to be a copy-paste error. It should indicate a failure to create the component (e.g., 'Failed to create component:'). - Reason this comment was not posted:
Marked as duplicate.
12. apps/studio/src/lib/editor/engine/projectinfo/index.ts:90
- Draft comment:
Typographical error in error logging: In the deleteComponent method, the catch block logs 'Failed to duplicate page:' which is inconsistent with the delete operation. It should be updated to something like 'Failed to delete component:'. - Reason this comment was not posted:
Marked as duplicate.
13. apps/studio/src/routes/editor/LayersPanel/ComponentsTab.tsx:83
- Draft comment:
Typo: In the handleDelete function, the error message 'Failed to duplicate page:' appears to be a copy-paste mistake. It should likely read 'Failed to delete component:' to accurately reflect the delete operation. - Reason this comment was not posted:
Marked as duplicate.
14. apps/studio/src/routes/editor/LayersPanel/ComponentsTab.tsx:95
- Draft comment:
Typo: In the handleDuplicate function, the error message 'Failed to duplicate page:' may be inaccurate. Consider updating it to 'Failed to duplicate component:' to clearly indicate the operation that failed. - Reason this comment was not posted:
Marked as duplicate.
15. packages/models/src/constants/ipc.ts:58
- Draft comment:
There is a typographical error in the enum constant name 'CLEAN_CODmemberE_KEYS'. It appears to be a mistake; it probably should be 'CLEAN_CODE_KEYS' to maintain consistency and clarity. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_JfAIcIjdtjjUZlkW
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.
Description
Close: #1602
This PR Introduces a new component tab and enables component creation functionality.
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Adds component tab and functionality for creating, renaming, duplicating, and deleting components in the editor.
components.ts
.code.ts
to handle new IPC channels for component operations.ComponentsTab.tsx
and integrates withLayersPanel
.LeftTabValue
enum inmodels.ts
for tab management.DUPLICATE_COMPONENT
,RENAME_COMPONENT
,CREATE_COMPONENT
,DELETE_COMPONENT
channels inipc.ts
.EditorEngine
andProjectInfoManager
to manage component operations and state.This description was created by
for 06147e1. It will automatically update as commits are pushed.