Skip to content

enterprise-util: Do not use app.quit() in case of renderer process. #1430

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

shubham-padia
Copy link
Member

@shubham-padia shubham-padia commented Jun 18, 2025

For renderer process, we will call ipcRenderer.send("quit-app") instead.

Fixes:

Screenshots and screen captures:

Platforms this PR was tested on:

  • Windows
  • macOS
  • Linux (specify distro)
Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

For renderer process, we will call `ipcRenderer.send("quit-app")`
instead.
@timabbott
Copy link
Member

Have you tested this in a way that confirms this works while the solution in #1425 did not?

@@ -6,6 +6,8 @@ import process from "node:process";
import {z} from "zod";
import {dialog} from "zulip:remote";

import {ipcRenderer} from "../renderer/js/typed-ipc-renderer.js";
Copy link
Member

Choose a reason for hiding this comment

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

This is worse. We should not import either main-only APIs or renderer-only APIs from common. Our linter configuration is supposed to enforce this, but that enforcement appears to be broken somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why this is worse. Can you explain why? Thanks :)

Copy link
Member

Choose a reason for hiding this comment

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

#1425 incorrectly imports a main-only API into common. This incorrectly imports both a main-only API and also a renderer-only API into common. We should import neither here, not both.

I understand that you think you’re avoiding issues by checking process.type, but (1) a linter cannot automatically verify that you did that check correctly, (2) it’s too easy to forget to check manually, and (3) the import itself has side effects that can’t be conditionalized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants