Skip to content

Conversation

FranjoMindek
Copy link
Contributor

@FranjoMindek FranjoMindek commented Aug 20, 2025

Updates discord.js from v12 to v14.
This also updates Discord API from v8 to v10.
Discord API v8 is deprecated.

Logic behind changes is explain on the changed files themselves.

@@ -91,6 +90,5 @@ function printAllTimeMonthlyReportCsvInCLI(
}

cliReport().catch((e) => {
const message = getAnalyticsErrorMessage(e);
Copy link
Contributor Author

@FranjoMindek FranjoMindek Aug 20, 2025

Choose a reason for hiding this comment

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

We should let the logger get the error object rather than message.
It has built in formatters to handle errors itself. No need to do it ourselves.

@@ -22,22 +22,21 @@ function isReportsChannel(channel: Discord.Channel): boolean {
}

export async function handleAnalyticsCommand(
discordClient: Discord.Client,
Copy link
Contributor Author

@FranjoMindek FranjoMindek Aug 20, 2025

Choose a reason for hiding this comment

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

We receive the Discord.Client object inside of the message object.
No need to pass it by ourselves.

Moreover, message.client'sDiscord.Client is sure to be ready (Discord.Client<true>).
This ensures some types don't have the optional flag anymore.

if (isOurDiscordBotMessage(discordClient, message)) {
return;
}
await discordClient.login(BOT_TOKEN);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is async, we should await and make start an async function.
Then the caller can decide if to await or not.
Like this the caller has no choice to await.

This comment was marked as resolved.

This comment was marked as resolved.

await handleAnalyticsCommand(discordClient, message);
}
async function handleGuildMessage(message: Discord.Message): Promise<void> {
if (message.author.bot) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't handle any bot. That includes this bot itself.

@FranjoMindek FranjoMindek self-assigned this Aug 20, 2025
embed.setImage(report.imageChartsChart.toURL());
options.embed = embed;
options.embeds = [embed];
Copy link
Contributor Author

@FranjoMindek FranjoMindek Aug 21, 2025

Choose a reason for hiding this comment

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

discord.js now supports up to 10 embeds, hence plural.

await reportsChannel.send(
`Failed to send daily analytics report: ${message}`,
`Failed to send daily analytics report. Check the logs for more details.
https://fly-metrics.net/d/fly-logs/fly-logs?orgId=273532&var-app=wasp-bot`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message was never really to helpful.
I've added a link to our wasp-bot graphana so we can easily check the logs.

This comment was marked as resolved.

Discord.GatewayIntentBits.Guilds,
Discord.GatewayIntentBits.GuildMessages,
Discord.GatewayIntentBits.GuildMembers,
Discord.GatewayIntentBits.MessageContent,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to add those intents to our bot's settings on Discord's developers portal.
Otherwise we will error on startup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Classic comment, but I'd add this to the code itself for the future when people update the code to go and update the settings.

@FranjoMindek FranjoMindek marked this pull request as ready for review August 21, 2025 08:24
Copy link
Member

@cprecioso cprecioso left a comment

Choose a reason for hiding this comment

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

Overall LGTM, left some comments

if (isOurDiscordBotMessage(discordClient, message)) {
return;
}
await discordClient.login(BOT_TOKEN);

This comment was marked as resolved.

@FranjoMindek FranjoMindek requested a review from cprecioso August 21, 2025 10:16
Copy link
Contributor

@infomiho infomiho left a comment

Choose a reason for hiding this comment

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

Left some comments

Discord.GatewayIntentBits.Guilds,
Discord.GatewayIntentBits.GuildMessages,
Discord.GatewayIntentBits.GuildMembers,
Discord.GatewayIntentBits.MessageContent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Classic comment, but I'd add this to the code itself for the future when people update the code to go and update the settings.


await handleMessage(newMessage);
});
// Messages sent in DM channels
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Messages sent in DM channels
// Ignore messages sent in the DM channels

],
});

discordClient.on(Discord.Events.ClientReady, (readyDiscordClient) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
discordClient.on(Discord.Events.ClientReady, (readyDiscordClient) => {
discordClient.on(Discord.Events.ClientReady, (startedDiscordClient) => {

Nitpick: the ready adjective can also read as a verb so it confused me for a second if this is a function or a "Discord client that is ready", so I think we maybe can improve the name here. Offered my suggestion above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I use ready since that is what discord.js uses, and I didn't want to introduce another domain context. But since this is really local we could go with started.

return !!member?.roles.cache.get(GUEST_ROLE_ID);
async function isGuestUser(message: GuildMessage): Promise<boolean> {
const member = await message.guild.members.fetch(message.author.id);
return Boolean(member.roles.resolve(GUEST_ROLE_ID));
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit feels a little bit convoluted, what is the value of member.roles.resolve(GUEST_ROLE_ID)? Is it something or null and that's why we wrap it into Boolean to get an implicit conversion to false if null?

I'd go for something explicit like member.roles.resolve(GUEST_ROLE_ID) !== null.

-readonly [K in keyof T]: T[K];
};

export type DeepWritable<T> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If not used, maybe we can remove this util?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reason is that it makes it easier to understand Writable is not recursive.
We can remove.

const options: Discord.MessageOptions = {};
): Discord.MessageCreateOptions {
const options: Discord.MessageCreateOptions = {};
const embeds: Writable<Discord.MessageCreateOptions["embeds"]> = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This threw me off a bit since it's removing readonly for an array of something and I expected Writeable to work on objects only. But I guess this is the way to do it since you need their type that readonly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this one is hard to do.
Shame they didn't extract their "files" and "embeds" into separate types before making them readonly.

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.

3 participants