-
Notifications
You must be signed in to change notification settings - Fork 46
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: modified discord i/o in the core package #62
base: main
Are you sure you want to change the base?
feat: modified discord i/o in the core package #62
Conversation
…o hide node_modules folders.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces enhancements to the Discord I/O module by expanding the Changes
Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/core/src/core/io/discord.ts (5)
30-34
: Consider extending ReactionData to include optional metadata.
This interface looks good for basic reaction data, but you might consider optionally including the channel or guild ID if your application needs to correlate where the reaction occurred.
36-40
: Consider including additional membership data.
Currently, you're capturing the userId, username, and whether they joined or left. If your bot needs more advanced details (like a member’s display name or roles), you might extendGuildMemberData
accordingly.
55-61
: Consider adding a "rename" or "update" action to ChannelData.
Discord channels can be renamed or changed, so if you need to monitor or capture these events, consider extending theaction
union to include them.
108-110
: Optional log for missing bot information.
The new null check forthis.client?.user
is good defensive coding. Consider logging a warning ifthis.client?.user
is undefined, to aid debugging.if (this.client?.user) { eventCallbacks.messageCreate(this.client.user, message); +} else { + this.logger.warn("DiscordClient", "Client user not found on messageCreate"); }
343-349
: Reference clarity in example usage.
The example referencescore
, but the usage details forcore.createMessageInput
andcore.createServerMemberInput
are not shown. If this is an intentional snippet, consider linking or briefly explaining the usage to avoid confusion for new contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/core/io/discord.ts
(7 hunks)
🔇 Additional comments (5)
packages/core/src/core/io/discord.ts (5)
48-53
: RoleData looks good.
No immediate concerns in this interface.
78-78
:isInitialized
is never updated.
The propertyisInitialized
staysfalse
throughout. Consider setting this totrue
upon successful client login or removing it if no longer used.
126-126
: Graceful client destruction.
Using optional chaining is a good safeguard. This line looks good.
130-145
: Review concurrency for createMessageInput.
The handler immediately returns the result ofmonitorMessages()
, which attaches a collector or event listener for messages but does not await its completion. This might yield incomplete data if the function is interpreted as capturing messages over a time window.
327-337
: Validate partial messages.
If this bot interacts with partial messages or ephemeral messages, ensure fields likeauthor
,channel
orcontent
exist. Otherwise, this method may throw errors on partial data.
packages/core/src/core/io/discord.ts
Outdated
private monitorServerMembers() { | ||
try { | ||
this.logger.debug( | ||
"DiscordClient.monitorServerMembers", | ||
"Monitoring server members" | ||
); | ||
|
||
const serverMembers: GuildMemberData[] = []; | ||
|
||
this.client?.on("guildMemberAdd", (member) => { | ||
serverMembers.push({ | ||
userId: member.id, | ||
username: member.user.username, | ||
action: "join", | ||
}); | ||
}); | ||
|
||
this.client?.on("guildMemberRemove", (member) => { | ||
serverMembers.push({ | ||
userId: member.id, | ||
username: member.user.username, | ||
action: "leave", | ||
}); | ||
}); | ||
|
||
return serverMembers; | ||
} catch (error) { | ||
this.logger.error( | ||
"DiscordClient.monitorGuildMembers", | ||
"Error monitoring guild members", | ||
{ | ||
error, | ||
} | ||
); | ||
throw error; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Immediate return of an empty array in monitorServerMembers().
Events are handled asynchronously, while serverMembers
is returned synchronously, resulting in empty data. If you need these joined/left events after the fact, consider a callback/promise-based approach, or store them over time in a shared state.
fa4969b
to
910079b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/core/src/core/io/discord.ts (1)
50-55
: Document permission requirements for role management.The
RoleData
interface handles role modifications. Consider adding JSDoc comments to document the required bot permissions for role management operations.+/** + * Interface for role modifications. + * Note: Requires 'MANAGE_ROLES' permission and the bot's highest role + * must be above the role being modified. + */ export interface RoleData { guildId: string; userId: string; roleId: string; action: "add" | "remove"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/core/io/discord.ts
(7 hunks)
🔇 Additional comments (1)
packages/core/src/core/io/discord.ts (1)
110-112
: LGTM! Improved null safety.Good improvements:
- Added null check for client.user before callback
- Added optional chaining in destroy method
Also applies to: 128-129
packages/core/src/core/io/discord.ts
Outdated
private formatMessageData(message: Message): MessageData { | ||
return { | ||
content: message.content, | ||
channelId: message.channel.id, | ||
sendBy: message.author.id, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix method binding in formatMessageData.
The formatMessageData
method is used as a callback in map
operations but isn't properly bound to the class instance.
-private formatMessageData(message: Message): MessageData {
+private formatMessageData = (message: Message): MessageData => {
return {
content: message.content,
channelId: message.channel.id,
sendBy: message.author.id,
};
-}
+};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private formatMessageData(message: Message): MessageData { | |
return { | |
content: message.content, | |
channelId: message.channel.id, | |
sendBy: message.author.id, | |
}; | |
} | |
private formatMessageData = (message: Message): MessageData => { | |
return { | |
content: message.content, | |
channelId: message.channel.id, | |
sendBy: message.author.id, | |
}; | |
}; |
private async monitorGuildMembers(): Promise<GuildMemberData> { | ||
return new Promise((resolve, reject) => { | ||
try { | ||
this.logger.debug( | ||
"DiscordClient.monitorGuildMembers", | ||
"Monitoring guild members" | ||
); | ||
|
||
const handleMemberEvent = ( | ||
member: GuildMember, | ||
action: "join" | "leave" | ||
) => { | ||
const memberData: GuildMemberData = { | ||
userId: member.id, | ||
username: member.user.username, | ||
action, | ||
}; | ||
resolve(memberData); | ||
}; | ||
|
||
this.client?.on("guildMemberAdd", (member) => | ||
handleMemberEvent(member as GuildMember, "join") | ||
); | ||
this.client?.on("guildMemberRemove", (member) => | ||
handleMemberEvent(member as GuildMember, "leave") | ||
); | ||
} catch (error) { | ||
this.logger.error( | ||
"DiscordClient.monitorGuildMembers", | ||
"Error monitoring guild members", | ||
{ error } | ||
); | ||
reject(error); | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix memory leak in guild member monitoring.
The monitorGuildMembers
method doesn't clean up event listeners, potentially causing memory leaks.
private async monitorGuildMembers(): Promise<GuildMemberData> {
return new Promise((resolve, reject) => {
try {
this.logger.debug(
"DiscordClient.monitorGuildMembers",
"Monitoring guild members"
);
+ const cleanup = () => {
+ this.client?.off("guildMemberAdd", handleMemberAdd);
+ this.client?.off("guildMemberRemove", handleMemberRemove);
+ };
+
+ const handleMemberAdd = (member: GuildMember) => {
+ cleanup();
+ handleMemberEvent(member, "join");
+ };
+
+ const handleMemberRemove = (member: GuildMember) => {
+ cleanup();
+ handleMemberEvent(member, "leave");
+ };
const handleMemberEvent = (
member: GuildMember,
action: "join" | "leave"
) => {
const memberData: GuildMemberData = {
userId: member.id,
username: member.user.username,
action,
};
resolve(memberData);
};
- this.client?.on("guildMemberAdd", (member) =>
- handleMemberEvent(member as GuildMember, "join")
- );
- this.client?.on("guildMemberRemove", (member) =>
- handleMemberEvent(member as GuildMember, "leave")
- );
+ this.client?.on("guildMemberAdd", handleMemberAdd);
+ this.client?.on("guildMemberRemove", handleMemberRemove);
+
+ // Add timeout to prevent hanging
+ setTimeout(() => {
+ cleanup();
+ reject(new Error('Guild member monitoring timed out'));
+ }, 60000);
} catch (error) {
+ cleanup?.();
this.logger.error(
"DiscordClient.monitorGuildMembers",
"Error monitoring guild members",
{ error }
);
reject(error);
}
});
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private async monitorGuildMembers(): Promise<GuildMemberData> { | |
return new Promise((resolve, reject) => { | |
try { | |
this.logger.debug( | |
"DiscordClient.monitorGuildMembers", | |
"Monitoring guild members" | |
); | |
const handleMemberEvent = ( | |
member: GuildMember, | |
action: "join" | "leave" | |
) => { | |
const memberData: GuildMemberData = { | |
userId: member.id, | |
username: member.user.username, | |
action, | |
}; | |
resolve(memberData); | |
}; | |
this.client?.on("guildMemberAdd", (member) => | |
handleMemberEvent(member as GuildMember, "join") | |
); | |
this.client?.on("guildMemberRemove", (member) => | |
handleMemberEvent(member as GuildMember, "leave") | |
); | |
} catch (error) { | |
this.logger.error( | |
"DiscordClient.monitorGuildMembers", | |
"Error monitoring guild members", | |
{ error } | |
); | |
reject(error); | |
} | |
}); | |
} | |
private async monitorGuildMembers(): Promise<GuildMemberData> { | |
return new Promise((resolve, reject) => { | |
try { | |
this.logger.debug( | |
"DiscordClient.monitorGuildMembers", | |
"Monitoring guild members" | |
); | |
const cleanup = () => { | |
this.client?.off("guildMemberAdd", handleMemberAdd); | |
this.client?.off("guildMemberRemove", handleMemberRemove); | |
}; | |
const handleMemberAdd = (member: GuildMember) => { | |
cleanup(); | |
handleMemberEvent(member, "join"); | |
}; | |
const handleMemberRemove = (member: GuildMember) => { | |
cleanup(); | |
handleMemberEvent(member, "leave"); | |
}; | |
const handleMemberEvent = ( | |
member: GuildMember, | |
action: "join" | "leave" | |
) => { | |
const memberData: GuildMemberData = { | |
userId: member.id, | |
username: member.user.username, | |
action, | |
}; | |
resolve(memberData); | |
}; | |
this.client?.on("guildMemberAdd", handleMemberAdd); | |
this.client?.on("guildMemberRemove", handleMemberRemove); | |
// Add timeout to prevent hanging | |
setTimeout(() => { | |
cleanup(); | |
reject(new Error('Guild member monitoring timed out')); | |
}, 60000); | |
} catch (error) { | |
cleanup?.(); | |
this.logger.error( | |
"DiscordClient.monitorGuildMembers", | |
"Error monitoring guild members", | |
{ error } | |
); | |
reject(error); | |
} | |
}); | |
} |
packages/core/src/core/io/discord.ts
Outdated
private async monitorMessages(channelId?: string): Promise<MessageData[]> { | ||
try { | ||
this.logger.debug( | ||
"DiscordClient.monitorMessages", | ||
"Monitoring messages", | ||
{ channelId } | ||
); | ||
|
||
const messages: Message[] = []; | ||
|
||
if (channelId) { | ||
// Fetch the channel and validate it | ||
const channel = await this.client?.channels.fetch(channelId); | ||
if (!channel || !channel.isTextBased()) { | ||
throw new Error( | ||
"Invalid channel or channel is not text-based" | ||
); | ||
} | ||
|
||
const textChannel = channel as TextChannel; | ||
|
||
// Create a promise that resolves when the message collector ends | ||
const collectedMessages = await new Promise<Message[]>( | ||
(resolve) => { | ||
const messageCollector = | ||
textChannel.createMessageCollector({ | ||
time: 60000, // Collect messages for 60 seconds | ||
}); | ||
|
||
messageCollector.on("collect", (message) => { | ||
messages.push(message); | ||
}); | ||
|
||
messageCollector.on("end", () => { | ||
this.logger.debug( | ||
"DiscordClient.monitorMessages", | ||
"Collected messages", | ||
{ count: messages.length } | ||
); | ||
resolve(messages); // Resolve the promise with the collected messages | ||
}); | ||
} | ||
); | ||
|
||
return collectedMessages.map(this.formatMessageData); | ||
} else { | ||
// Handle global message collection (no specific channel) | ||
return new Promise<MessageData[]>((resolve) => { | ||
const messageHandler = (message: Message) => { | ||
messages.push(message); | ||
}; | ||
|
||
this.client?.on("messageCreate", messageHandler); | ||
|
||
// Optionally, you can add a timeout or condition to stop listening | ||
setTimeout(() => { | ||
this.client?.off("messageCreate", messageHandler); // Stop listening | ||
resolve(messages.map(this.formatMessageData)); // Resolve with collected messages | ||
}, 60000); // Stop after 60 seconds | ||
}); | ||
} | ||
} catch (error) { | ||
this.logger.error( | ||
"DiscordClient.monitorMessages", | ||
"Error monitoring messages", | ||
{ error } | ||
); | ||
throw error; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix race conditions and improve message monitoring.
Several issues in the monitorMessages
method need attention:
- Magic number (60000) is duplicated
- Race condition from previous review still exists
- No cleanup for global message collection
+private readonly DEFAULT_COLLECTION_TIMEOUT = 60000;
+
private async monitorMessages(channelId?: string): Promise<MessageData[]> {
try {
this.logger.debug(
"DiscordClient.monitorMessages",
"Monitoring messages",
{ channelId }
);
const messages: Message[] = [];
+ const cleanup = () => {
+ if (!channelId) {
+ this.client?.off("messageCreate", messageHandler);
+ }
+ };
if (channelId) {
// ... channel validation ...
// Create a promise that resolves when the message collector ends
const collectedMessages = await new Promise<Message[]>(
(resolve) => {
const messageCollector =
textChannel.createMessageCollector({
- time: 60000, // Collect messages for 60 seconds
+ time: this.DEFAULT_COLLECTION_TIMEOUT,
});
messageCollector.on("collect", (message) => {
messages.push(message);
});
messageCollector.on("end", () => {
this.logger.debug(
"DiscordClient.monitorMessages",
"Collected messages",
{ count: messages.length }
);
resolve(messages);
});
}
);
- return collectedMessages.map(this.formatMessageData);
+ return collectedMessages.map((m) => this.formatMessageData(m));
} else {
// Handle global message collection
- return new Promise<MessageData[]>((resolve) => {
+ return new Promise<MessageData[]>((resolve, reject) => {
const messageHandler = (message: Message) => {
messages.push(message);
};
this.client?.on("messageCreate", messageHandler);
setTimeout(() => {
- this.client?.off("messageCreate", messageHandler);
- resolve(messages.map(this.formatMessageData));
- }, 60000);
+ cleanup();
+ resolve(messages.map((m) => this.formatMessageData(m)));
+ }, this.DEFAULT_COLLECTION_TIMEOUT);
+
+ // Handle promise rejection
+ setTimeout(() => {
+ cleanup();
+ reject(new Error('Message collection timed out'));
+ }, this.DEFAULT_COLLECTION_TIMEOUT + 1000);
});
}
} catch (error) {
+ cleanup?.();
this.logger.error(
"DiscordClient.monitorMessages",
"Error monitoring messages",
{ error }
);
throw error;
}
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private async monitorMessages(channelId?: string): Promise<MessageData[]> { | |
try { | |
this.logger.debug( | |
"DiscordClient.monitorMessages", | |
"Monitoring messages", | |
{ channelId } | |
); | |
const messages: Message[] = []; | |
if (channelId) { | |
// Fetch the channel and validate it | |
const channel = await this.client?.channels.fetch(channelId); | |
if (!channel || !channel.isTextBased()) { | |
throw new Error( | |
"Invalid channel or channel is not text-based" | |
); | |
} | |
const textChannel = channel as TextChannel; | |
// Create a promise that resolves when the message collector ends | |
const collectedMessages = await new Promise<Message[]>( | |
(resolve) => { | |
const messageCollector = | |
textChannel.createMessageCollector({ | |
time: 60000, // Collect messages for 60 seconds | |
}); | |
messageCollector.on("collect", (message) => { | |
messages.push(message); | |
}); | |
messageCollector.on("end", () => { | |
this.logger.debug( | |
"DiscordClient.monitorMessages", | |
"Collected messages", | |
{ count: messages.length } | |
); | |
resolve(messages); // Resolve the promise with the collected messages | |
}); | |
} | |
); | |
return collectedMessages.map(this.formatMessageData); | |
} else { | |
// Handle global message collection (no specific channel) | |
return new Promise<MessageData[]>((resolve) => { | |
const messageHandler = (message: Message) => { | |
messages.push(message); | |
}; | |
this.client?.on("messageCreate", messageHandler); | |
// Optionally, you can add a timeout or condition to stop listening | |
setTimeout(() => { | |
this.client?.off("messageCreate", messageHandler); // Stop listening | |
resolve(messages.map(this.formatMessageData)); // Resolve with collected messages | |
}, 60000); // Stop after 60 seconds | |
}); | |
} | |
} catch (error) { | |
this.logger.error( | |
"DiscordClient.monitorMessages", | |
"Error monitoring messages", | |
{ error } | |
); | |
throw error; | |
} | |
} | |
private readonly DEFAULT_COLLECTION_TIMEOUT = 60000; | |
private async monitorMessages(channelId?: string): Promise<MessageData[]> { | |
try { | |
this.logger.debug( | |
"DiscordClient.monitorMessages", | |
"Monitoring messages", | |
{ channelId } | |
); | |
const messages: Message[] = []; | |
const cleanup = () => { | |
if (!channelId) { | |
this.client?.off("messageCreate", messageHandler); | |
} | |
}; | |
if (channelId) { | |
// Fetch the channel and validate it | |
const channel = await this.client?.channels.fetch(channelId); | |
if (!channel || !channel.isTextBased()) { | |
throw new Error( | |
"Invalid channel or channel is not text-based" | |
); | |
} | |
const textChannel = channel as TextChannel; | |
// Create a promise that resolves when the message collector ends | |
const collectedMessages = await new Promise<Message[]>( | |
(resolve) => { | |
const messageCollector = | |
textChannel.createMessageCollector({ | |
time: this.DEFAULT_COLLECTION_TIMEOUT, | |
}); | |
messageCollector.on("collect", (message) => { | |
messages.push(message); | |
}); | |
messageCollector.on("end", () => { | |
this.logger.debug( | |
"DiscordClient.monitorMessages", | |
"Collected messages", | |
{ count: messages.length } | |
); | |
resolve(messages); | |
}); | |
} | |
); | |
return collectedMessages.map((m) => this.formatMessageData(m)); | |
} else { | |
// Handle global message collection (no specific channel) | |
return new Promise<MessageData[]>((resolve, reject) => { | |
const messageHandler = (message: Message) => { | |
messages.push(message); | |
}; | |
this.client?.on("messageCreate", messageHandler); | |
setTimeout(() => { | |
cleanup(); | |
resolve(messages.map((m) => this.formatMessageData(m))); | |
}, this.DEFAULT_COLLECTION_TIMEOUT); | |
// Handle promise rejection | |
setTimeout(() => { | |
cleanup(); | |
reject(new Error('Message collection timed out')); | |
}, this.DEFAULT_COLLECTION_TIMEOUT + 1000); | |
}); | |
} | |
} catch (error) { | |
cleanup?.(); | |
this.logger.error( | |
"DiscordClient.monitorMessages", | |
"Error monitoring messages", | |
{ error } | |
); | |
throw error; | |
} | |
} |
910079b
to
98ee460
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/core/src/core/io/discord.ts (3)
189-254
:⚠️ Potential issueFix race conditions and improve message monitoring.
The message monitoring implementation still has potential issues with cleanup and race conditions.
The previous review provided a comprehensive fix that addresses:
- Proper cleanup of event listeners
- Race condition handling
- Timeout management
- Error handling improvements
Please refer to the previous review comment for the detailed implementation.
305-340
:⚠️ Potential issueFix memory leak in guild member monitoring.
The implementation needs proper cleanup of event listeners and timeout handling.
The previous review provided a comprehensive fix that addresses:
- Event listener cleanup
- Timeout handling
- Error handling improvements
Please refer to the previous review comment for the detailed implementation.
341-347
:⚠️ Potential issueFix method binding in formatMessageData.
The method needs proper binding to work correctly when used as a callback.
The previous review provided a fix using arrow function syntax. Please refer to that review comment for the implementation.
🧹 Nitpick comments (1)
packages/core/src/core/io/discord.ts (1)
50-63
: Consider adding optional fields for better extensibility.The interfaces could benefit from additional optional fields that might be useful for future features:
export interface RoleData { guildId: string; userId: string; roleId: string; action: "add" | "remove"; + roleName?: string; // Role name can be useful for logging/debugging + timestamp?: number; // When the role change occurred } export interface ChannelData { guildId: string; action: "create" | "delete"; name?: string; type?: ChannelType; channelId?: string; + parentId?: string; // For nested channels + position?: number; // Channel position in the guild }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/core/io/discord.ts
(7 hunks)
🔇 Additional comments (2)
packages/core/src/core/io/discord.ts (2)
110-112
: LGTM! Improved null safety.The added null check for
this.client?.user
enhances the safety of the messageCreate event handling.
161-166
: Fix type mismatch in response schema.The response schema's
action
field is typed as "string" but should match GuildMemberData's "join" | "leave" type.response: { type: "string", userId: "string", username: "string", - action: "string", + action: { type: "string", enum: ["join", "leave"] }, },
fix: initialize goals without dependencies as ready
…tions feat: add context to CoT actions for cleaner memory output
…-settings chore: updated vscode settings and added recommended extensions
feat: update IO and improve types, update discord example
nice work - i have made some changes to the types for easier subscriptions - could you update this PR to match |
@ponderingdemocritus can you please take a look if it looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
examples/example-api.ts (1)
Consider adding URL restrictions for security
The current implementation allows any valid URL format, which could pose security risks:
- No domain allowlist/blocklist mechanisms
- No URL sanitization beyond basic format validation
- Potential for SSRF (Server-Side Request Forgery) attacks
Consider:
- Adding a domain allowlist using environment variables
- Implementing URL sanitization middleware
- Adding domain pattern validation in the schema
🔗 Analysis chain
Line range hint
117-127
: LGTM! Well-documented API handler.The schema provides clear documentation for API calls. However, let's verify the security implications of allowing any URL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for URL validation or sanitization rg -A 5 "url\.toString\(\)|new URL\(|validateUrl|sanitizeUrl"Length of output: 63
Script:
#!/bin/bash # Search for URL-related configurations and security checks rg -i "allowedUrls|allowedDomains|urlPattern|baseUrl|apiUrl|endpoint" -A 3Length of output: 9344
Script:
#!/bin/bash # Search for fetch/axios usage and security middleware rg -i "fetch\(|axios\.|middleware.*url|validateRequest" -A 5Length of output: 4979
Script:
#!/bin/bash # Look for security-related configurations fd -t f "config|security|env" --exec cat {}Length of output: 22002
🧹 Nitpick comments (12)
.vscode/settings.json (1)
7-8
: Consider consolidating Prettier settings.While the configuration is correct, consider moving the
singleQuote
setting toprettier.config.cjs
for better maintainability, as it's generally preferred to keep all Prettier configurations in one place.{ "editor.defaultFormatter": "esbenp.prettier-vscode", "editor.formatOnSave": true, "files.exclude": { "**/node_modules": true }, "prettier.configPath": "prettier.config.cjs", - "prettier.singleQuote": false, "search.exclude": { "**/node_modules": true } }
examples/example-discord.ts (3)
49-49
: MongoDB scheduling note.
Clear note on scheduled tasks usage. Make sure to handle MongoDB connection failures gracefully.
83-94
: Real-time streaming input registration.
This approach to subscribe and stream messages intoOrchestrator
is straightforward and flexible.
Consider adding minimal error handling or logging if the callback fails to process the message.
123-123
: Extra handler removal.
discord_reply
is not explicitly registered in the current snippet. Confirm the correct name of the output handler or remove if unused.examples/example-api.ts (1)
181-185
: Consider enhancing logging structure.While the implementation is correct, consider using structured logging for better debugging and monitoring capabilities.
- console.log(`Reply to user ${userId ?? "??"}: ${message}`); + console.log({ + event: "ui_chat_reply", + userId: userId ?? "??", + message + });packages/core/src/core/processors/research-processor.ts (1)
Line range hint
202-215
: LGTM! Consider adding type annotations for better readability.The changes correctly handle the new handler types and safely access the outputSchema.
Consider adding explicit return type annotation:
- private buildHandlerSchemaPart( - handlers?: OutputIOHandler[] | ActionIOHandler[] - ): string { + private buildHandlerSchemaPart( + handlers?: OutputIOHandler[] | ActionIOHandler[] + ): string {packages/core/src/core/types/index.ts (4)
550-557
: Consider adding input type parameter for better type safety.The interface looks good, but could benefit from generic type parameters.
Consider adding a generic type parameter for input data:
-export interface InputIOHandler extends BaseIOHandler { +export interface InputIOHandler<T = any> extends BaseIOHandler { role: HandlerRole.INPUT; - execute?: (data: any) => Promise<unknown>; - subscribe?: (onData: (data: any) => void) => () => void; + execute?: (data: T) => Promise<unknown>; + subscribe?: (onData: (data: T) => void) => () => void; }
576-585
: Consider adding output type parameter for better type safety.The interface looks good with required schema validation.
Consider adding a generic type parameter for output data:
-export interface OutputIOHandler extends BaseIOHandler { +export interface OutputIOHandler<T = any> extends BaseIOHandler { role: HandlerRole.OUTPUT; - outputSchema: z.ZodType<any>; - execute?: (data: any) => Promise<unknown>; - subscribe?: (onData: (data: any) => void) => () => void; + outputSchema: z.ZodType<T>; + execute?: (data: T) => Promise<unknown>; + subscribe?: (onData: (data: T) => void) => () => void; }
601-608
: Consider adding input/output type parameters for better type safety.The interface looks good with optional schema validation.
Consider adding generic type parameters for input and output data:
-export interface ActionIOHandler extends BaseIOHandler { +export interface ActionIOHandler<TInput = any, TOutput = any> extends BaseIOHandler { role: HandlerRole.ACTION; - outputSchema?: z.ZodType<any>; - execute?: (data: any) => Promise<unknown>; + outputSchema?: z.ZodType<TInput>; + execute?: (data: TInput) => Promise<TOutput>; }
610-611
: Update union type if adding generic type parameters.The union type correctly combines all handler types.
If you implement the suggested generic type parameters, update the union type:
-export type IOHandler = InputIOHandler | OutputIOHandler | ActionIOHandler; +export type IOHandler = + | InputIOHandler<any> + | OutputIOHandler<any> + | ActionIOHandler<any, any>;packages/core/src/core/orchestrator.ts (1)
124-134
: Consider using logger instead of console.log.The cleanup sequence is good, but logging could be improved.
Replace console.log with the logger:
- console.log(`Removed IOHandler: ${name}`); + this.logger.info( + "Orchestrator.removeIOHandler", + "Removed IOHandler", + { name } + );packages/core/src/core/io/discord.ts (1)
Line range hint
222-293
: Consider implementing rate limiting.While the error handling is thorough, Discord has rate limits that should be handled explicitly to prevent temporary bans.
+private messageQueue: Array<{ data: MessageData; resolve: Function; reject: Function }> = []; +private isProcessing = false; +private readonly RATE_LIMIT = 1000; // 1 second between messages +private async processMessageQueue() { + if (this.isProcessing || this.messageQueue.length === 0) return; + this.isProcessing = true; + + const { data, resolve, reject } = this.messageQueue.shift()!; + try { + const result = await this._sendMessage(data); + resolve(result); + } catch (error) { + reject(error); + } + + this.isProcessing = false; + setTimeout(() => this.processMessageQueue(), this.RATE_LIMIT); +} private async sendMessage(data: MessageData) { + return new Promise((resolve, reject) => { + this.messageQueue.push({ data, resolve, reject }); + this.processMessageQueue(); + }); } +private async _sendMessage(data: MessageData) { // Move existing sendMessage implementation here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.vscode/extensions.json
(1 hunks).vscode/settings.json
(1 hunks)docs/docs/pages/index.mdx
(6 hunks)docs/vocs.config.ts
(1 hunks)examples/example-api.ts
(5 hunks)examples/example-basic.ts
(3 hunks)examples/example-discord.ts
(3 hunks)examples/example-goal.ts
(3 hunks)examples/example-server.ts
(1 hunks)examples/example-twitter.ts
(4 hunks)packages/core/src/core/chain-of-thought.ts
(7 hunks)packages/core/src/core/io/discord.ts
(6 hunks)packages/core/src/core/orchestrator.ts
(8 hunks)packages/core/src/core/processors/message-processor.ts
(4 hunks)packages/core/src/core/processors/research-processor.ts
(4 hunks)packages/core/src/core/types/index.ts
(2 hunks)prettier.config.cjs
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/extensions.json
🔇 Additional comments (53)
prettier.config.cjs (2)
12-16
: LGTM! Well-configured YAML formatting options.The YAML configuration with
proseWrap: "preserve"
andbracketSpacing: false
follows standard practices for YAML file formatting, improving readability while maintaining existing line wrapping.
23-24
: LGTM! Good Markdown formatting configuration.The Markdown settings with
proseWrap: "always"
andembeddedLanguageFormatting: "auto"
are ideal for maintaining consistent documentation formatting and proper code block handling..vscode/settings.json (3)
2-3
: LGTM! Good editor formatting configuration.The configuration ensures consistent code formatting across the team by using Prettier and enabling format on save.
4-6
: LGTM! Proper exclusion of node_modules.Correctly excludes
node_modules
from both file explorer and search results, which improves VS Code performance.Also applies to: 9-11
12-12
: LGTM! Proper TypeScript SDK configuration.Using the local TypeScript installation ensures consistency across the team.
examples/example-discord.ts (17)
2-4
: Great explanatory doc comments.
These updated comments clearly describe the new streaming approach for handling Discord messages.
8-8
: Imports look consistent.
Pulling inHandlerRole
andLogLevel
aligns well with the changes for advanced logging and role specification.
20-20
: Explicit typing for messages.
ImportingMessage
fromdiscord.js
is appropriate for strongly-typed callback parameters.
23-23
: Concise log level comment.
The inline note ensures developers can easily adjust the verbosity for debugging.
32-33
: Purge usage.
Purging thevectorDb
on every run is helpful for testing a fresh session, but confirm if this is intentional for production.
38-38
: Model selection.
Using"anthropic/claude-3-5-sonnet-latest"
is specific. Ensure the environment supports this model and correct usage quotas.
42-42
: Processor comment.
Describing the default character config helps new contributors quickly grasp the setup.
58-58
: Clearing tasks.
Purging tasks is a good way to ensure no leftover state. Confirm it’s not accidental if you want persistent tasks in a real environment.
61-61
: Orchestrator creation.
The constructor parameters look consistent and well-organized for the streaming approach.
74-74
: Discord client initialization.
Establishing the client here is a logical placement in the setup sequence.
78-80
: Bot name & log level injection.
Includingdiscord_bot_name
is a neat addition for identification; passingloglevel
ensures consistent verbosity.
99-99
: Output handler comment.
The comment clarifies the function of the output handler for Discord messages.
102-102
: Message output registration.
discord.createMessageOutput()
presumably defaults to a unique name, but ensure it aligns with the name removed on shutdown (discord_reply
).
111-113
: Console logs for user instructions.
Helpful logs guide developers on how to interact with the process.
114-114
: Exit tip.
Indicating the exit command is a helpful user experience detail.
116-116
: Graceful shutdown comment.
Reminding contributors to handle Ctrl-C is beneficial for safe resource cleanup.
120-122
: Removing streaming IO handlers.
Properly cleaning up these handlers avoids memory leaks, but confirm bothdiscord_stream
anddiscord_reply
exist if you remove them.docs/vocs.config.ts (1)
4-4
: Updated Discord link.
Ensure the new invitation URL (https://discord.gg/rt8ajxQvXh
) remains valid.examples/example-basic.ts (2)
89-93
: Renaming for clarity.
Switching fromhandler
/schema
toexecute
/outputSchema
improves function clarity. Confirm documentation references match the new property names.
Line range hint
115-128
: GraphQL output logic.
Renaminghandler
toexecute
andschema
tooutputSchema
aligns with the updated naming convention. The typed object helps keep queries safe.packages/core/src/core/processors/message-processor.ts (2)
3-9
: LGTM! Type safety improvements.The introduction of specific handler types (
ActionIOHandler
andOutputIOHandler
) and their usage in theprocess
method parameters improves type safety and better represents the distinct roles of different handlers.Also applies to: 53-54
106-110
: LGTM! Clear response guidelines.The new thinking block provides clear criteria for when the processor should reply, which helps prevent unnecessary responses and spam.
examples/example-api.ts (2)
96-99
: LGTM! Consistent handler registration.The changes align with the core type updates, using the new
outputSchema
property andexecute
method name.
166-172
: LGTM! Clean input handler.The input handler correctly passes through the payload without modification, maintaining the expected contract.
examples/example-server.ts (2)
78-82
: LGTM! Consistent WebSocket handler.The handler changes maintain consistency with other examples while properly integrating with WebSocket functionality.
90-94
: Consider consistent logging improvements.Similar to the ui_chat_reply handler, this could benefit from structured logging for consistency across the codebase.
examples/example-twitter.ts (4)
95-95
: LGTM! Robust Twitter integration.The handler properly handles Twitter mentions with appropriate error cases and null returns when no mentions are found.
121-121
: LGTM! Clean thought generation handler.The handler maintains a clean separation of concerns for autonomous thought generation with proper error handling.
138-145
: LGTM! Well-validated Twitter output.The handler includes proper content validation using regex and provides clear schema documentation.
167-172
: LGTM! Well-structured reply handler.The handler properly handles optional reply-to functionality with clear schema documentation.
packages/core/src/core/processors/research-processor.ts (2)
221-223
: LGTM! Type definitions properly updated.The parameter types correctly use the new handler interfaces, improving type safety.
326-327
: LGTM! Type definitions properly updated.The parameter types correctly use the new handler interfaces, maintaining consistency with the rest of the changes.
packages/core/src/core/types/index.ts (1)
531-534
: LGTM! Well-structured base interface.The BaseIOHandler interface provides a clean foundation for all IO handlers.
examples/example-goal.ts (2)
107-115
: LGTM! Handler properly renamed to execute.The changes correctly align with the new interface while maintaining the same functionality.
Line range hint
137-150
: LGTM! Handler properly renamed to execute.The changes correctly align with the new interface while maintaining the same functionality.
packages/core/src/core/orchestrator.ts (5)
81-81
: LGTM! Good addition for cleanup management.The unsubscribers Map helps prevent memory leaks by properly managing subscription cleanup.
98-111
: LGTM! Well-implemented subscription handling.The changes properly handle input handler subscriptions with appropriate logging and cleanup management.
Line range hint
143-162
: LGTM! Properly updated to use execute method.The changes correctly handle the new interface while maintaining good error handling.
182-189
: LGTM! Good error handling for missing execute method.The changes properly validate the execute method with appropriate error logging.
537-538
: LGTM! Clear error handling for missing execute method.The changes properly validate the execute method with a descriptive error message.
packages/core/src/core/io/discord.ts (5)
Line range hint
1-62
: Well-structured interfaces and schema implementation!The new interfaces and zod schema provide good type safety and validation. The message schema particularly helps ensure data consistency.
165-171
: Mismatch between GuildMemberData and handler response.The response schema uses string type for action, while GuildMemberData defines it as "join" | "leave".
177-187
: Excellent type-safe message output implementation!The generic type parameter and zod schema validation provide strong type safety and runtime validation.
295-330
: Fix memory leak in guild member monitoring.The event listeners are not properly cleaned up, which could cause memory leaks.
332-375
: Solid role management implementation!Good error handling, proper role validation, and DRY_RUN support make this implementation robust.
packages/core/src/core/chain-of-thought.ts (2)
1066-1066
: Good type safety improvements!The changes properly enforce the OutputIOHandler type and its required properties (execute and outputSchema).
Also applies to: 1086-1087, 1092-1095
1222-1228
: Improved action documentation.The documentation for available actions is now clearer with the addition of context requirements.
docs/docs/pages/index.mdx (1)
88-93
: Documentation accurately reflects code changes.The updated examples and explanations align well with the new code structure and type safety improvements.
Also applies to: 165-175
@@ -57,13 +63,13 @@ export class MessageProcessor extends BaseProcessor { | |||
|
|||
const outputsSchemaPart = ioContext?.availableOutputs | |||
.map((handler) => { | |||
return `${handler.name}: ${JSON.stringify(zodToJsonSchema(handler.schema, handler.name))}`; | |||
return `${handler.name}: ${JSON.stringify(zodToJsonSchema(handler.outputSchema!, handler.name))}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Schema nullability assertion needs partial revision
The non-null assertion is safe for OutputIOHandler
as outputSchema
is required, but should be guarded for ActionIOHandler
where it's optional:
packages/core/src/core/processors/message-processor.ts:66
- Safe for output handlerspackages/core/src/core/processors/message-processor.ts:72
- Needs null check for action handlers
🔗 Analysis chain
Verify schema nullability handling.
The use of the non-null assertion operator (!
) with handler.outputSchema
might be risky. Consider adding runtime checks or making the schema required in the type definition.
Also applies to: 72-72
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for type definition of OutputIOHandler and ActionIOHandler
ast-grep --pattern 'interface $_(IOHandler) {
$$$
outputSchema$_
$$$
}'
Length of output: 79
Script:
#!/bin/bash
# Try multiple search strategies
# 1. Search for interfaces with broader pattern
ast-grep --pattern 'interface $_ {
$$$
schema
$$$
}'
# 2. Search for handler type definitions
rg "type.*Handler.*=.*{" -A 5
# 3. Look for actual handler implementations
rg "outputSchema.*=.*" -A 2
# 4. Search for files containing both Handler and Schema
fd . -e ts -e tsx --exec grep -l "Handler" {} \; | xargs grep -l "Schema"
Length of output: 678
Script:
#!/bin/bash
# Examine type definitions and implementations
# Check type definitions
cat packages/core/src/core/types/index.ts
# Look for handler implementations in message processor
cat packages/core/src/core/processors/message-processor.ts
# Search for schema-related patterns in both files
rg "outputSchema|schema.*:" -A 2 "packages/core/src/core/types/index.ts" "packages/core/src/core/processors/message-processor.ts"
Length of output: 26751
if ( | ||
message.author.displayName == this.credentials.discord_bot_name | ||
) { | ||
console.log( | ||
`Skipping message from ${this.credentials.discord_bot_name}` | ||
); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use bot ID instead of display name for message filtering.
Using displayName
for bot message filtering is unreliable as display names can be changed. Consider using the bot's ID instead:
-if (message.author.displayName == this.credentials.discord_bot_name) {
+if (message.author.id === this.client.user?.id) {
console.log(`Skipping message from ${this.credentials.discord_bot_name}`);
return;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if ( | |
message.author.displayName == this.credentials.discord_bot_name | |
) { | |
console.log( | |
`Skipping message from ${this.credentials.discord_bot_name}` | |
); | |
return; | |
} | |
if ( | |
message.author.id === this.client.user?.id | |
) { | |
console.log( | |
`Skipping message from ${this.credentials.discord_bot_name}` | |
); | |
return; | |
} |
private async monitorVoiceStates() { | ||
try { | ||
this.logger.debug( | ||
"DiscordClient.monitorVoiceStates", | ||
"Monitoring voice states" | ||
); | ||
|
||
const voiceStates: VoiceStateData[] = []; | ||
|
||
this.client.on("voiceStateUpdate", (oldState, newState) => { | ||
if (oldState.channelId !== newState.channelId) { | ||
voiceStates.push({ | ||
userId: newState.id, | ||
channelId: newState.channelId || "unknown", | ||
action: oldState.channelId | ||
? newState.channelId | ||
? "move" | ||
: "leave" | ||
: "join", | ||
}); | ||
} | ||
}); | ||
|
||
return voiceStates; | ||
} catch (error) { | ||
this.logger.error( | ||
"DiscordClient.monitorVoiceStates", | ||
"Error monitoring voice states", | ||
{ | ||
error, | ||
} | ||
); | ||
throw error; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix voice state monitoring implementation.
The current implementation has two issues:
- Event listeners are not cleaned up
- The method returns immediately while events are handled asynchronously
private async monitorVoiceStates(): Promise<VoiceStateData[]> {
- try {
- this.logger.debug(
- "DiscordClient.monitorVoiceStates",
- "Monitoring voice states"
- );
-
- const voiceStates: VoiceStateData[] = [];
-
- this.client.on("voiceStateUpdate", (oldState, newState) => {
- if (oldState.channelId !== newState.channelId) {
- voiceStates.push({
- userId: newState.id,
- channelId: newState.channelId || "unknown",
- action: oldState.channelId
- ? newState.channelId
- ? "move"
- : "leave"
- : "join",
- });
- }
- });
-
- return voiceStates;
+ return new Promise((resolve, reject) => {
+ try {
+ this.logger.debug(
+ "DiscordClient.monitorVoiceStates",
+ "Monitoring voice states"
+ );
+
+ const voiceStates: VoiceStateData[] = [];
+ let timeout: NodeJS.Timeout;
+
+ const handleVoiceUpdate = (oldState: any, newState: any) => {
+ if (oldState.channelId !== newState.channelId) {
+ voiceStates.push({
+ userId: newState.id,
+ channelId: newState.channelId || "unknown",
+ action: oldState.channelId
+ ? newState.channelId
+ ? "move"
+ : "leave"
+ : "join",
+ });
+ }
+ };
+
+ this.client.on("voiceStateUpdate", handleVoiceUpdate);
+
+ // Resolve after collecting events for a period
+ timeout = setTimeout(() => {
+ this.client.off("voiceStateUpdate", handleVoiceUpdate);
+ resolve(voiceStates);
+ }, 60000); // Collect for 60 seconds
+
+ // Handle errors
+ this.client.on("error", (error) => {
+ clearTimeout(timeout);
+ this.client.off("voiceStateUpdate", handleVoiceUpdate);
+ reject(error);
+ });
} catch (error) {
this.logger.error(
"DiscordClient.monitorVoiceStates",
"Error monitoring voice states",
{ error }
);
- throw error;
+ reject(error);
}
+ });
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private async monitorVoiceStates() { | |
try { | |
this.logger.debug( | |
"DiscordClient.monitorVoiceStates", | |
"Monitoring voice states" | |
); | |
const voiceStates: VoiceStateData[] = []; | |
this.client.on("voiceStateUpdate", (oldState, newState) => { | |
if (oldState.channelId !== newState.channelId) { | |
voiceStates.push({ | |
userId: newState.id, | |
channelId: newState.channelId || "unknown", | |
action: oldState.channelId | |
? newState.channelId | |
? "move" | |
: "leave" | |
: "join", | |
}); | |
} | |
}); | |
return voiceStates; | |
} catch (error) { | |
this.logger.error( | |
"DiscordClient.monitorVoiceStates", | |
"Error monitoring voice states", | |
{ | |
error, | |
} | |
); | |
throw error; | |
} | |
} | |
private async monitorVoiceStates(): Promise<VoiceStateData[]> { | |
return new Promise((resolve, reject) => { | |
try { | |
this.logger.debug( | |
"DiscordClient.monitorVoiceStates", | |
"Monitoring voice states" | |
); | |
const voiceStates: VoiceStateData[] = []; | |
let timeout: NodeJS.Timeout; | |
const handleVoiceUpdate = (oldState: any, newState: any) => { | |
if (oldState.channelId !== newState.channelId) { | |
voiceStates.push({ | |
userId: newState.id, | |
channelId: newState.channelId || "unknown", | |
action: oldState.channelId | |
? newState.channelId | |
? "move" | |
: "leave" | |
: "join", | |
}); | |
} | |
}; | |
this.client.on("voiceStateUpdate", handleVoiceUpdate); | |
// Resolve after collecting events for a period | |
timeout = setTimeout(() => { | |
this.client.off("voiceStateUpdate", handleVoiceUpdate); | |
resolve(voiceStates); | |
}, 60000); // Collect for 60 seconds | |
// Handle errors | |
this.client.on("error", (error) => { | |
clearTimeout(timeout); | |
this.client.off("voiceStateUpdate", handleVoiceUpdate); | |
reject(error); | |
}); | |
} catch (error) { | |
this.logger.error( | |
"DiscordClient.monitorVoiceStates", | |
"Error monitoring voice states", | |
{ error } | |
); | |
reject(error); | |
} | |
}); | |
} |
Potentially closes #23
Summary by CodeRabbit
New Features
Improvements
Documentation