Skip to content

Add strong typing #4

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add strong typing #4

wants to merge 3 commits into from

Conversation

soraxas
Copy link

@soraxas soraxas commented May 22, 2025

Thanks for the cool extension!

Just an opinionated draft PR to correct some of the incorrect typing I've seen in the console (it fixed a few errors where this.mail doesn't exist due to Mail.fromAuthor(..) doesn't always returns valid mail.

In my mailbox, I've seen cases where there are email in messagesAuthorsSet that contains no email address <...> from the author string, which results in no way to get subdomain/tld/etc. That should not be a valid Mail object.

These are just some preliminary type fixes that I want to create before other PR on adding some functionality. The extension is also currently very laggy, and sometimes display incorrect icon on email header (I'm using ThunderbirdConvension) which I intent to investigate later on

soraxas added 3 commits May 23, 2025 01:02
@noam-sc
Copy link
Owner

noam-sc commented May 22, 2025

Thanks for this great contribution !

I really appreciate the strong typing and linting, I was thinking about adding it. On a side note, I'm also working on adding unit tests to make the code more robust.

I'm aware of the ghost emails issue in Thunderbird (https://bugzilla.mozilla.org/show_bug.cgi?id=752237). As I experienced it, the extension manages it (probably in a wobbly way). I'm not sure if it is the bug you are describing but I think it should be adressed together.

Sometimes the author is just an email address without any name (and the <...>) but it still remains a valid email.

You're right, there is indeed room for performance improvements in the whole extension, especially on the Thunderbird UI management, which is quite complex to understand (and I haven't understood it fully)

@@ -32,16 +32,20 @@ export default class Mail {
if (email) {
return email[1].toLowerCase().trim();
}
return author.toLowerCase().trim();
return null;
Copy link
Owner

Choose a reason for hiding this comment

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

If we give the function just a simple email ("[email protected]") without a name and <..>, it will return null even though it is a valid email

Copy link
Author

@soraxas soraxas May 23, 2025

Choose a reason for hiding this comment

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

Oh I see what you mean.

I actually have not encountered that. When I encountered the exception, i printed messagesAuthorsSet and it mostly looks something like

["First Last <[email protected]>", "Foo foo bar bar <[email protected]>", ...]

and then the problematic one is

[..., "Business School blahblah", ...]

where it does not has the <...> and only has the name of the author. I think it's a mailing list? (even though in the GUI it does display the correct address).


I've also had cases where the author is an empty string "" when I browse the draft folder (which make sense, as there's no author). That also causes an exception.

Copy link
Author

Choose a reason for hiding this comment

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

Regarding your case, maybe that happens if the sender does not assign any sender name, and Thunderbird would directly display the sender address?

Perhaps one way is to do a regex parse on the author string? Can thunderbird extension use npm package? It seems https://github.com/jackbearheart/email-addresses would be a good choice, as it uses RFC3522 standard to parse the author string, and gives back name, address, domain, etc.

Copy link
Owner

Choose a reason for hiding this comment

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

If the GUI manages to display the correct email, it means we can retrieve it in the email headers. I think it can be handled in the getCorrespondent function

async getCorrespondent(message, context = "inboxList") {

We'll have to look into draft emails indeed.

Regarding my case, I noticed that there is no sender name from Yahoo emails and some automatic noreply emails (which represents for me a lot of emails).

Thunderbird extensions cannot use npm packages, they need to be bundled in, like the ICAL library. The library you mentioned is interesting, I'll check it!

Copy link
Author

Choose a reason for hiding this comment

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

Replied in the main thread, but there's actually no from address. Looking at the UI, perhaps we should have some robust way to extract author name + sender_address. If it fails, perhaps it should fallback to using the reply-to address, and re-use the same logic of getting icon from domain/subdomain/etc.

I think these might happens to the following scenario

  • email is sending on-behalf of someone, where sender and the intended receiver are different, e.g. managed account
  • mailing list, where sender is some bot mailman address but the reply-to target is a human account

@soraxas
Copy link
Author

soraxas commented May 23, 2025

Thanks for the quick review! I would be happy to help out, as I'd like my thunderbird to look pretty:) (I was using Mailspring which had a good implementation of these icon usage)

Regarding the bug, in my previous testing, there were some exceptions in the Mail class (

getDomain() {
const split = this.mail.split("@");
if (split.length < 2) {
console.warn("Invalid email", this.mail, this.author);
// Ghost mails https://bugzilla.mozilla.org/show_bug.cgi?id=752237
return "";
}
return this.mail.split("@")[1];
}
) when it tries to split this.mail (where this.mail is null). And I traced it that it was due to that mail created from Mail.fromAuthor, where mail as null.

@noam-sc
Copy link
Owner

noam-sc commented May 23, 2025

We have a common objective :)

If you have an example to share of an email headers (or how to receive such an email) triggering this bug, that would be helpful!

@soraxas
Copy link
Author

soraxas commented May 24, 2025

Hey @noam-sc after double checking, there's actually only a reply-to address and no from address in the GUI.

This is my console log of everything that (after my PR) contains no author (i.e. no author found)
Screenshot 2025-05-24 at 5 32 52 pm
(as you've mentioned, that includes many no-reply address).

*Notice that the last entry contains NO address, only a name.

and this is the UI
Screenshot 2025-05-24 at 5 31 15 pm

which, normally should contains a from: field to indicate the sender address.


After peeking into the eml, this is what was shown somewhere in the file:

...
Received: by outmail5.placeholder.com id h1hfo0380ogl for <[email protected]>; Mon, 14 Jan 2025 11:01:58 +0100 (envelope-from <[email protected]>)
To: "" <[email protected]>
Subject: Blahblah, Innovation and Entrepreneurship Seminar 
From: InnovationandEntrepreneurship=?UTF-8?B?4oCT?=TheUBlahblahplaceholder
Reply-To: [email protected]
X-SD-JobID: zzzz670c59b4b
X-SD-User: zzzz58473e089b
x-job: zzzz670c59b4b9d2
X-SD-Publicator: PublicatorBatch
...

@noam-sc
Copy link
Owner

noam-sc commented May 24, 2025

Thanks for taking the time to check it.

Here is what I'm thinking :

  • In the case the author has no email (only the name), then check for the reply-to email. If present, build a new author name looking like Sender's name <reply to email>. I guess this fallback system would be implemented here :
    async getCorrespondent(message, context = "inboxList") {
  • In the case the author has no email and no reply-to email, still build a "Mail" object (whose name should be Author actually) so as to still get the initial displayed along its associated background color. (your example should display I as an initial if there was no reply-to email)

What do you think of that ?

@soraxas
Copy link
Author

soraxas commented May 25, 2025

That's all good!

Yea I agree that perhaps that'd be quite a good fallback approach.

But in the second case (where no from and reply-to), if we still construct a Mail object, the current logic might still be subject to the strategies of getting domain/remove subdomain/etc. Since it has no address, that means it will still has the problem that i described (this.mail being null)?

@soraxas
Copy link
Author

soraxas commented May 25, 2025

On a different note (since you've mentioned initials), another thing that I was going to raise in issue/PR is, perhaps we should use 2 initials as its less ambiguous? (using 2 initials is very common in other mail app, mostly refers to first/last name).

I saw that you've implemented creating the initial image, which is very cool. But have you come across https://ui-avatars.com/?

You can directly create an initial avatar using it, and it had implemented logics of selecting 2 initials to display (e.g. it will use First+Last name if possible, otherwise, it will use the first 2 letter if the name only contains 1 word)

You only need to do

"https://ui-avatars.com/api/?name=" + encodeURIComponent(author)

e.g.

// firstname lastname
"https://ui-avatars.com/api/?name=John+Doe"

// firstname middlename middlename lastname
"https://ui-avatars.com/api/?name=John+Middle+AnotherMiddle+Doe"

// only first name
"https://ui-avatars.com/api/?name=John"

// with color
"https://ui-avatars.com/api/?name=John+Doe&background=995555&color=ffffff"

@noam-sc
Copy link
Owner

noam-sc commented May 25, 2025

That's all good!

Yea I agree that perhaps that'd be quite a good fallback approach.

But in the second case (where no from and reply-to), if we still construct a Mail object, the current logic might still be subject to the strategies of getting domain/remove subdomain/etc. Since it has no address, that means it will still has the problem that i described (this.mail being null)?

Yes you're right, there will be a problem if the email is null. But that could be fixed by adding a method hasEmail() like hasAName() in the Mail/Author object. Then, before applying strategies in ProfilePictureFetcher, we can check if there is an email (and that would be the only place where we need to check)

On a different note (since you've mentioned initials), another thing that I was going to raise in issue/PR is, perhaps we should use 2 initials as its less ambiguous? (using 2 initials is very common in other mail app, mostly refers to first/last name).

I saw that you've implemented creating the initial image, which is very cool. But have you come across https://ui-avatars.com/?

You can directly create an initial avatar using it, and it had implemented logics of selecting 2 initials to display (e.g. it will use First+Last name if possible, otherwise, it will use the first 2 letter if the name only contains 1 word)

Displaying two initials is a great idea. I originally stuck with Thunderbird default UI of initials (which are shown without Thunderbird Conversations), which only shows one initial. Adding this as an option would be really nice. The different background colors also help reduce ambiguity.

For ui-avatars online solution, I'm not a fan for several reasons :

  • It would create unnecessary network requests
  • It would require a new storage structure to distinguish "initials" images from actual avatars to allow missing avatars to be periodically refetched
  • The current UI implementation of initials is consistent with Thunderbird UI
  • It would require a new offline fallback strategy
  • I'm not comfortable with the idea of sending all correspondent names to a third-party server (even though they claim they don't store anything)
  • The logic you're mentioning could be implemented directly within the addon

@soraxas
Copy link
Author

soraxas commented May 25, 2025

Yep sounds good with both!

Regarding the ui-avatar, yeah, it'd be better to be able to delegate the complexity to another 3rd party library, but the extra network request is true, and since most have already been implemented, let's reuse the existing functionality.

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