Skip to content

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?

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