Testingbranch#9
Conversation
WalkthroughAdds type refinements and small API-surface typings: introduces internal MessageParams handling, exports EventHelpers type, updates plugin event typings, improves Discord event typing and partial scheduled-event delete handling, calls shutdownRateLimiter during shutdown, and performs a tiny plugin router cast refactor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 OverviewGreptile SummaryImproved type safety across the plugin system by replacing
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Main as index.ts
participant PM as PluginManager
participant Plugin as HoloPlugin
participant Router as Express Router
participant EventBus as PluginEventBus
participant Discord as Discord Client
Main->>PM: setContext(client, io, config, app)
Main->>PM: loadPlugins()
PM->>Plugin: import plugin file
PM->>PM: validate metadata
alt Plugin has events hook
PM->>EventBus: createEventHelpers(eventBus)
PM->>Plugin: events(helpers, ctx)
Plugin-->>PM: return EventSubscription[]
PM->>PM: store subscriptions
end
alt Plugin has routes hook
PM->>Router: create plugin sub-router
PM->>Plugin: routes(wrappedRouter, ctx)
Plugin->>Router: register GET/POST/etc endpoints
PM->>Router: mount at /api/plugins/{name}
end
PM->>Plugin: onLoad(ctx)
PM->>EventBus: emitPlugin('plugin:loaded', {name, version})
Note over Discord,EventBus: Runtime: Discord events flow
Discord->>PM: emit(eventName, data)
PM->>EventBus: emitDiscord(eventName, data)
EventBus->>Plugin: trigger event handlers
Note over Main,PM: Shutdown sequence
Main->>Main: shutdownRateLimiter()
Main->>PM: unloadAll()
PM->>EventBus: unsubscribe all subscriptions
PM->>Plugin: onUnload()
PM->>EventBus: emitPlugin('plugin:unloaded', {name})
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/api/routes/messages.ts(1 hunks)src/discord/events/index.ts(3 hunks)src/index.ts(2 hunks)src/plugins/manager.ts(1 hunks)src/plugins/sdk.ts(1 hunks)src/types/plugin.types.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/types/plugin.types.ts (1)
src/plugins/sdk.ts (1)
EventHelpers(173-173)
src/discord/events/index.ts (1)
src/discord/serializers.ts (2)
serializeUser(67-79)serializeScheduledEvent(441-466)
src/index.ts (1)
src/api/middleware/rateLimit.ts (1)
shutdownRateLimiter(156-162)
🔇 Additional comments (7)
src/index.ts (1)
5-5: LGTM! Proper resource cleanup during shutdown.The rate limiter cleanup correctly clears intervals and the rate limit store. The shutdown order is appropriate—cleaning up the rate limiter before unloading plugins and exiting.
Also applies to: 48-49
src/plugins/manager.ts (1)
217-219: LGTM! Cleaner alternative to non-null assertion.The cast is safe since
pluginSubRouteris guaranteed non-null within theif (plugin.routes)block where it was initialized at line 173. This refactor improves readability.src/discord/events/index.ts (2)
269-281: LGTM! Safe type assertion with partial check.The type assertion is appropriately guarded by checking
oldUser.partial === falsebefore serializing.
608-622: The type assertion onnewEventis safe given the current configuration.Since
Partials.GuildScheduledEventis not enabled in the client configuration (src/discord/client.ts:39-46), Discord.js guarantees thatnewEventwill never be partial inguildScheduledEventUpdateevents. The cast on line 619 is valid. However, ifPartials.GuildScheduledEventis ever added to the client config in the future, this code would break—serializeScheduledEventaccesses properties (e.g.,event.coverImageURL()on line 463) that would be undefined on partial objects. Consider adding an explicit.partialcheck for consistency witholdEventand future-proofing against configuration changes.src/plugins/sdk.ts (1)
170-173: LGTM! Proper type export for public API.Exporting
EventHelpersas a type alias enables consistent typing across the codebase and provides a stable public API for plugin authors.src/api/routes/messages.ts (1)
5-22: LGTM! Improved type safety with clear documentation.The
MessageParamsinterface and updatedgetParamshelper provide better type safety. The JSDoc comment clearly explains thatchannelIdis merged from the parent router viamergeParams: true, which justifies the empty string fallback.src/types/plugin.types.ts (1)
12-12: LGTM! Type consolidation improves consistency.Replacing the inline helpers object type with the
EventHelperstype exported fromsrc/plugins/sdk.tscentralizes the type definition and ensures consistency across the plugin API surface.Also applies to: 94-97
… and their payloads.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/types/events.types.ts (1)
563-574: Union type for partial scheduled-event deletes looks correct; document consumer expectations.The
SerializedScheduledEvent | { id; guildId; partial: true }union cleanly models the new minimal delete payload and aligns with the broadcaster. It’s a breaking type tightening for consumers expecting full data onguildScheduledEventDelete, so it’s worth calling out in plugin/client docs so they always branch onpartial === truebefore accessing other fields.src/discord/events/index.ts (2)
269-281: userUpdate: partial guard + typed serialization is sound.The
oldUser.partial ? null : serializeUser(oldUser as User)pattern is consistent with your other partial handling and keepsserializeUseron fully-hydrated instances only. Looks good.
608-621: guildScheduledEventUpdate typing improvements are correct and consistent.Guarding
oldEventwith!oldEvent.partialbefore serializing and assertingGuildScheduledEventmatches the serializer’s expectations and mirrors your userUpdate pattern. If you ever see partialnewEvents in practice, you might later mirror that guard for symmetry, but current logic is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/discord/events/index.ts(3 hunks)src/types/events.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/types/events.types.ts (1)
src/types/discord.types.ts (1)
SerializedScheduledEvent(243-266)
src/discord/events/index.ts (2)
src/discord/serializers.ts (2)
serializeUser(67-79)serializeScheduledEvent(441-466)src/discord/client.ts (1)
discordClient(9-47)
🔇 Additional comments (2)
src/discord/events/index.ts (2)
1-2: Type-only discord.js imports are appropriate here.Using
import type { User, GuildScheduledEvent }keeps these as purely compile-time aids and matches their usage in later assertions, so no runtime behavior change.
624-645: Partial scheduled-event delete handling correctly fixes the previous bug.Short-circuiting on
event.partialand broadcasting only{ id, guildId, partial: true }avoids unsafe calls intoserializeScheduledEventon incomplete data, and the non-partial path still serializes the fullGuildScheduledEvent. This aligns with the updatedGuildScheduledEventDeleteEvent.dataunion and resolves the earlier critical issue around delete handling.
Summary by CodeRabbit
Bug Fixes
Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.