Skip to content

Implemented the duplicate/delete web frame feature #1875

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

Closed
wants to merge 1 commit into from

Conversation

harjot20022001
Copy link
Contributor

@harjot20022001 harjot20022001 commented May 19, 2025

Description

Implemented the feature to duplicate and delete web frames on canvas.

Related Issues

None

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Release
  • Refactor
  • Other (please describe):

Testing

None

Screenshots (if applicable)

None

Additional Notes

None


Important

Implemented web frame duplication feature in WindowManager class with unique ID generation and position adjustment.

  • Feature:
    • Implemented web frame duplication in WindowManager class in index.ts.
    • duplicate() function creates a new WebFrame with a unique ID and adjusted position.
    • Logs duplication action with sendAnalytics.
  • Imports:
    • Added nanoid for generating unique IDs.
    • Added WebFrame type for frame handling.

This description was created by Ellipsis for bd69c99. You can customize this summary. It will automatically update as commits are pushed.

Copy link

vercel bot commented May 19, 2025

@harjot20022001 is attempting to deploy a commit to the Onlook Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to bd69c99 in 1 minute and 37 seconds. Click for details.
  • Reviewed 84 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 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/web/client/src/components/store/editor/window/index.ts:118
  • Draft comment:
    Remove or replace console.log with a proper logging mechanism for production.
  • 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% Console.log statements in production code are generally considered bad practice as they clutter the console and may expose internal details. However, this seems like debug logging that could be helpful during development. The codebase already uses console.error for error cases, suggesting there isn't an established logging framework. Without knowing the logging standards of this codebase, this feels like a minor stylistic suggestion rather than a critical issue. I may be underestimating the importance of proper logging in production. Console.log statements could expose sensitive information or impact performance. While proper logging is important, this comment doesn't provide specific guidance on what logging mechanism to use, and without knowing the project's standards, removing debug logs could make development harder. The comment should be deleted as it's more of a general best practice suggestion rather than a specific, actionable issue with the changes. The debug logs aren't clearly problematic without more context about the project's logging standards.
2. apps/web/client/src/components/store/editor/window/index.ts:146
  • Draft comment:
    Consider deep cloning windowMetadata if it is mutable to avoid shared state issues.
  • 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 shared mutable state can be problematic, we don't have enough context to know if windowMetadata is mutable or if sharing its reference would cause issues. We can't see the WebFrame type definition or how windowMetadata is used elsewhere. The suggestion is speculative without strong evidence that this is actually a problem. The comment raises a valid software engineering concern about mutable shared state. Deep cloning objects is a common defensive programming practice. However, without clear evidence that windowMetadata is mutable or that sharing its reference would cause problems, this comment is speculative and may suggest unnecessary defensive code. Delete the comment as it makes a speculative suggestion without strong evidence that there's actually a problem with the current implementation.
3. apps/web/client/src/components/store/editor/window/index.ts:149
  • Draft comment:
    If using MobX observable arrays, consider using .push(newFrame) instead of reassigning the array.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_6ulT8K39vUlDLA2s

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

@harjot20022001 harjot20022001 changed the title Implemented the duplicate web frame feature Implemented the duplicate/delete web frame feature May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant