Merging the new functionalities into the testing branch #6
Conversation
Added docs and multiple event types to the application including threads and bulk messages and some more
…API key security documentation, and include wget in Dockerfile
…oks for routes and events
…ord API routes, and plugin infrastructure.
… CRUD operations and Zod validation.
Fixing major and critical bugs, adding more support to the plug-in system, and improving a lot of ways to introduce a much more centralized way of developing plugins
Testingbranch
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptile review pls |
Greptile OverviewGreptile SummaryThis PR represents a major architectural upgrade to HoloBridge, transforming it from a basic Discord bridge into a comprehensive plugin platform with REST API capabilities. The changes introduce a comprehensive plugin SDK with REST route registration, typed event subscriptions through an event bus, and inter-plugin communication features. Core improvements include enhanced error handling across all Discord API routes with try-catch blocks, standardized error responses, and proper TypeScript type safety. The plugin system now supports mounting custom HTTP endpoints at Important Files Changed
Confidence score: 2/5
Sequence DiagramsequenceDiagram
participant User
participant CLI as "CLI (holo.js)"
participant Main as "Main (index.ts)"
participant Discord as "Discord Client"
participant Config as "Config Loader"
participant PluginMgr as "Plugin Manager"
participant APIServer as "API Server"
participant Middleware as "Auth Middleware"
participant Routes as "API Routes"
participant Services as "Discord Services"
User->>CLI: "holo start"
CLI->>Main: "Execute main()"
Main->>Config: "Load configuration"
Config-->>Main: "Return config with Discord token, API keys"
Main->>Discord: "loginDiscord()"
Discord-->>Main: "Connection established"
Main->>Discord: "waitForReady()"
Discord-->>Main: "Ready state confirmed"
Main->>APIServer: "createApiServer()"
APIServer-->>Main: "Return server instance with Express app, Socket.IO"
Main->>PluginMgr: "setContext(client, io, config, app)"
PluginMgr-->>Main: "Context initialized"
Main->>PluginMgr: "loadPlugins()"
PluginMgr->>PluginMgr: "Read plugins directory"
PluginMgr->>PluginMgr: "Load plugin files"
PluginMgr-->>Main: "Plugins loaded"
Main->>APIServer: "startApiServer()"
APIServer-->>Main: "Server listening on port"
Note over Main: "HoloBridge ready"
User->>APIServer: "API Request with X-API-Key"
APIServer->>Middleware: "authMiddleware()"
Middleware->>Config: "Validate API key"
Config-->>Middleware: "Key validation result"
Middleware-->>APIServer: "Authenticated request"
APIServer->>Routes: "Route handler"
Routes->>Services: "Discord service call"
Services->>Discord: "Discord API operation"
Discord-->>Services: "Discord response"
Services-->>Routes: "Serialized data"
Routes-->>APIServer: "JSON response"
APIServer-->>User: "API response"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const globalCleanupInterval = setInterval(() => { | ||
| const now = Date.now(); | ||
| for (const [key, entry] of rateLimitStore) { | ||
| if (entry.resetAt < now) { | ||
| rateLimitStore.delete(key); | ||
| } | ||
| } | ||
| }, 60000); | ||
| cleanupIntervals.push(globalCleanupInterval); |
There was a problem hiding this comment.
logic: the global cleanup interval runs immediately at module load time (before any server is started), creating a timer that runs forever even if the server is never started or fails to start
| const globalCleanupInterval = setInterval(() => { | |
| const now = Date.now(); | |
| for (const [key, entry] of rateLimitStore) { | |
| if (entry.resetAt < now) { | |
| rateLimitStore.delete(key); | |
| } | |
| } | |
| }, 60000); | |
| cleanupIntervals.push(globalCleanupInterval); | |
| // Track the global cleanup interval (initialized when server starts) | |
| let globalCleanupInterval: NodeJS.Timeout | null = null; | |
| /** | |
| * Initialize the global rate limiter cleanup interval. | |
| * Should be called when the server starts. | |
| */ | |
| export function initRateLimiter(): void { | |
| if (!globalCleanupInterval) { | |
| globalCleanupInterval = setInterval(() => { | |
| const now = Date.now(); | |
| for (const [key, entry] of rateLimitStore) { | |
| if (entry.resetAt < now) { | |
| rateLimitStore.delete(key); | |
| } | |
| } | |
| }, 60000); | |
| cleanupIntervals.push(globalCleanupInterval); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/api/middleware/rateLimit.ts
Line: 142:150
Comment:
**logic:** the global cleanup interval runs immediately at module load time (before any server is started), creating a timer that runs forever even if the server is never started or fails to start
```suggestion
// Track the global cleanup interval (initialized when server starts)
let globalCleanupInterval: NodeJS.Timeout | null = null;
/**
* Initialize the global rate limiter cleanup interval.
* Should be called when the server starts.
*/
export function initRateLimiter(): void {
if (!globalCleanupInterval) {
globalCleanupInterval = setInterval(() => {
const now = Date.now();
for (const [key, entry] of rateLimitStore) {
if (entry.resetAt < now) {
rateLimitStore.delete(key);
}
}
}, 60000);
cleanupIntervals.push(globalCleanupInterval);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| }; | ||
|
|
There was a problem hiding this comment.
logic: the loadedPlugins getter doesn't exist in the class definition, causing a runtime error
| }; | |
| listPlugins: () => Array.from(this.plugins.keys()), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/manager.ts
Line: 77:78
Comment:
**logic:** the `loadedPlugins` getter doesn't exist in the class definition, causing a runtime error
```suggestion
listPlugins: () => Array.from(this.plugins.keys()),
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Additional Comments (2)
-
src/discord/services/automod.service.ts, line 30-35 (link)style: inconsistent error handling - this method silently catches errors without logging, unlike other methods in this service. Should this method log errors like getAutoModRules and createAutoModRule do for consistency?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/discord/services/automod.service.ts, line 61-67 (link)style: inconsistent error handling - silently catches errors without logging, unlike getAutoModRules and createAutoModRule
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
28 files reviewed, 5 comments
| with: | ||
| # Upload entire repository | ||
| path: '.' |
There was a problem hiding this comment.
logic: uploading entire repository exposes all source code, environment files, and potential secrets publicly on GitHub Pages
| with: | |
| # Upload entire repository | |
| path: '.' | |
| with: | |
| # Upload only documentation directory | |
| path: 'docs' |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/static.yml
Line: 38:40
Comment:
**logic:** uploading entire repository exposes all source code, environment files, and potential secrets publicly on GitHub Pages
```suggestion
with:
# Upload only documentation directory
path: 'docs'
```
How can I resolve this? If you propose a fix, please make it concise.| onLoad(ctx) { | ||
| ctx.logger.info('Example plugin v2.0.0 loaded!'); | ||
| ctx.logger.info(`Connected to ${ctx.client.guilds.cache.size} guild(s)`); | ||
| ctx.logger.info(`Other plugins: ${ctx.listPlugins().join(', ') || 'none yet'}`); |
There was a problem hiding this comment.
logic: calls ctx.listPlugins() which references the non-existent loadedPlugins property in the plugin manager, causing a runtime error
Prompt To Fix With AI
This is a comment left during a code review.
Path: plugins/example-plugin.js
Line: 101:101
Comment:
**logic:** calls `ctx.listPlugins()` which references the non-existent `loadedPlugins` property in the plugin manager, causing a runtime error
How can I resolve this? If you propose a fix, please make it concise.| success: false, | ||
| error: 'Invalid request body', | ||
| code: 'VALIDATION_ERROR', | ||
| details: parseResult.error.issues, |
There was a problem hiding this comment.
style: inconsistent error response structure - uses 'details' instead of 'issues' like the POST route
| details: parseResult.error.issues, | |
| issues: parseResult.error.issues.map(issue => ({ | |
| path: issue.path.join('.'), | |
| message: issue.message, | |
| })), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/api/routes/automod.ts
Line: 157:157
Comment:
**style:** inconsistent error response structure - uses 'details' instead of 'issues' like the POST route
```suggestion
issues: parseResult.error.issues.map(issue => ({
path: issue.path.join('.'),
message: issue.message,
})),
```
How can I resolve this? If you propose a fix, please make it concise.
No description provided.