Skip to content
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 confirmation for opening external apps #1618

Merged
merged 6 commits into from
Nov 16, 2024

Conversation

ericyzhu
Copy link
Contributor

@ericyzhu ericyzhu commented Nov 15, 2024

Description

image

PR Type

  • Feature
  • Bugfix
  • Hotfix
  • Other (please describe):

Changelog

  • I have updated the changelog/next.md with my changes.

@follow-reviewer-bot
Copy link

Thank you for your contribution. We will review it promptly.

Copy link

vercel bot commented Nov 15, 2024

@ericyzhu is attempting to deploy a commit to the RSS3 Team on Vercel.

A member of the Team first needs to authorize it.

@follow-reviewer-bot
Copy link

Suggested PR Title:

feat: add external URL confirmation dialog for navigation

Change Summary:
Enhanced window navigation with external URL confirmation dialog and added localization for dialog messages.

Code Review:

  • File Path: apps/main/src/window.ts, Line 110-124: The webContents.on("will-navigate") event handler's logic to check the protocol is appropriate for avoiding potential security risks; however, it currently does not account for http and https URLs. The user will not be prompted and the e.preventDefault() will prevent navigation for these protocols. Ensure that desired behavior is allowed for http and https URLs:

    if (protocol === "http:" || protocol === "https:" || protocol === "follow:") {
      // Handle internal navigation for these protocols if needed
      return;
    }
  • File Path: apps/main/src/window.ts, Line 110-112: You should also unregister or clean up event listeners to prevent memory leaks. Ensure the listener is removed when the BrowserWindow is closed:

    window.on("closed", () => {
      webContents.removeAllListeners("will-navigate");
    });

These changes are necessary to ensure the code is robust, secure, and free from potential memory leaks.

Aside from the above points, everything else looks good!

If you make these changes, it should address the identified issues and enhance the overall quality of the code.

Thus, ensuring a thorough and well-rounded review.

@ericyzhu ericyzhu marked this pull request as draft November 16, 2024 05:09
@ericyzhu ericyzhu marked this pull request as ready for review November 16, 2024 05:17
Signed-off-by: Innei <tukon479@gmail.com>
Signed-off-by: Innei <tukon479@gmail.com>
@Innei
Copy link
Member

Innei commented Nov 16, 2024

@ericyzhu I modified some logic. Everything is working well.

@Innei Innei merged commit 08a1e76 into RSSNext:dev Nov 16, 2024
4 of 6 checks passed
@ericyzhu ericyzhu deleted the feat/open-external-apps-confirmation branch November 16, 2024 11:59
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.

2 participants