feat: New features in Portal #434
Conversation
|
I like the functionality that this PR brings but I can't really say it's something we'd accept at the moment We did our best to retain a functional programming style for Robo.js, hence the lack of builders and conventional OOP patterns. This keeps our overhead minimal and goes well with file-based frameworks like Robo. A controller pattern violates this. Another thing is, even though we have DJS coupled, we're actively trying to decouple DJS into its own thing away from the core. Adding this plus snowflakes in core APIs like Portal will make it harder to migrate later on. An ideal solution would be more like:
I'll keep your PR open because it has some good ideas, until we release this functionality using the above syntax. Until then, we'd appreciate it if you could split some parts of this PR into its own thing that we can accept now, such as the type enhancements |
|
@Pkmmte 👋 |
create-discord-activity
create-robo
@robojs/ai
@robojs/ai-voice
@robojs/server
@robojs/better-stack
@roboplay/plugin-confessions
@robojs/dev
@roboplay/plugin-gpt
@robojs/maintenance
@robojs/moderation
@roboplay/plugin-poll
@robojs/sync
robo.js
@roboplay/sage
@robojs/analytics
@robojs/auth
@robojs/cron
@robojs/i18n
@robojs/patch
@robojs/trpc
commit: |
WalkthroughAdds a portal-based enablement system with per-module/command/event/middleware/context flags and server-specific restrictions, enforces these checks across handlers, introduces portal controllers/utilities, extends types and config for keepRegistered, and updates help filtering and gitignore. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Handler as Handler Entry
participant Portal as Portal (enabledState)
participant Utils as portalUtils
participant Middleware as Middleware Chain
participant Logic as Business Logic
User->>Handler: invoke (command/autocomplete/context/event)
Handler->>Portal: check module enabled?
alt module disabled
Handler-->>User: return early
else module enabled
Handler->>Portal: check item enabled? (command/event/context)
alt item disabled
Handler-->>User: return early
else item enabled
Handler->>Portal: check server restriction for item
alt not enabled for server
Handler-->>User: return early
else allowed for server
Handler->>Middleware: iterate middleware
loop for each middleware
Middleware->>Utils: isEnabled + isEnabledForServer?
alt middleware disabled or blocked
Middleware-->>Handler: skip
else allowed
Middleware->>Logic: execute
end
end
Handler->>Logic: execute core handler
Logic-->>User: respond
end
end
end
sequenceDiagram
participant Manifest as Manifest Loader
participant Portal as Portal
participant Utils as portalUtils
Manifest->>Portal: load entry with serverOnly/disabled?
alt entry.serverOnly present
Manifest->>Utils: setServerOnly for module/entries
end
alt entry.disabled present
Manifest->>Portal: update enabledState for module/entries
end
Portal->>Portal: store keepRegistered from Config if present
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/robo/src/default/commands/help.ts (1)
304-346: Help interactions ignore availability filters.Category and pagination flows use the full command list, re‑exposing disabled or out‑of‑scope commands. Filter by availability here too.
Apply this diff:
const manifest = getManifest() - const commands = getInnermostCommands(manifest.commands) + const commands = getInnermostCommands(manifest.commands) + const serverId = interaction.guildId + const available = commands.filter((cmd) => { + const pc = cmd.key ? portal.command(cmd.key) : null + if (pc && !pc.isEnabled) return false + const pm = cmd.command.__module ? portal.module(cmd.command.__module) : null + if (pm && !pm.isEnabled) return false + if (serverId && cmd.key && !portal.isEnabledForServer(cmd.key, serverId)) return false + return true + }) @@ if (interaction.isStringSelectMenu()) { const selectedCategory = interaction.values[0] - const categorizedCommands = categorizeCommands(commands) + const categorizedCommands = categorizeCommands(available) const categories = Object.keys(categorizedCommands) - const filteredCommands = selectedCategory === 'all' ? commands : categorizedCommands[selectedCategory] || [] + const filteredCommands = selectedCategory === 'all' ? available : categorizedCommands[selectedCategory] || [] @@ if (interaction.isButton()) { let page = parseInt(parts[2], 10) || 0 const category = parts[4] === 'all' ? undefined : parts[4] - const categorizedCommands = categorizeCommands(commands) + const categorizedCommands = categorizeCommands(available) const categories = Object.keys(categorizedCommands) - const filteredCommands = category ? categorizedCommands[category] || [] : commands + const filteredCommands = category ? categorizedCommands[category] || [] : available
🧹 Nitpick comments (12)
packages/robo/src/core/portal-utils.ts (3)
34-41: Deduplicate and normalize server IDs on write.Prevent duplicates when setting server restrictions.
- setServerOnly: ( + setServerOnly: ( serverRestrictions: Record<string, string[]>, key: string, serverIds: string[] | string ): void => { - const servers = Array.isArray(serverIds) ? serverIds : [serverIds] + const servers = Array.isArray(serverIds) ? [...new Set(serverIds)] : [serverIds] serverRestrictions[key] = servers },
22-33: Optional: faster membership check.If lists grow, consider Sets for O(1) checks. Keep API the same, but convert on read.
- const restrictions = serverRestrictions[key] + const restrictions = serverRestrictions[key] if (!restrictions || restrictions.length === 0) { return true } - return restrictions.includes(serverId) + // Convert once per read; micro-opt: cache at a higher layer if hot + return new Set(restrictions).has(serverId)
100-143: Don’t overwrite explicit per-item restrictions when applying module-level ones.Preserve existing item-level settings; only fill missing keys. Also, avoid repeated
Globals.getPortalValues()calls.- applyModuleServerRestrictions: ( + applyModuleServerRestrictions: ( serverRestrictions: Record<string, string[]>, moduleName: string ): void => { - const servers = serverRestrictions[moduleName] + const servers = serverRestrictions[moduleName] if (!servers || servers.length === 0) return - - const commands = Globals.getPortalValues().commands + const portal = Globals.getPortalValues() + const commands = portal.commands if (commands) { commands.forEach((command, key) => { if (command.module === moduleName) { - serverRestrictions[key] = servers + if (!(key in serverRestrictions)) serverRestrictions[key] = servers } }) } - const events = Globals.getPortalValues().events + const events = portal.events if (events) { events.forEach((eventHandlers, eventName) => { eventHandlers.forEach((handler, index) => { if (handler.module === moduleName) { const key = `${eventName}:${index}` - serverRestrictions[key] = servers + if (!(key in serverRestrictions)) serverRestrictions[key] = servers } }) }) } - const middleware = Globals.getPortalValues().middleware + const middleware = portal.middleware middleware.forEach((mw, index) => { if (mw.module === moduleName) { - serverRestrictions[index.toString()] = servers + const key = index.toString() + if (!(key in serverRestrictions)) serverRestrictions[key] = servers } }) - const contexts = Globals.getPortalValues().context + const contexts = portal.context if (contexts) { contexts.forEach((context, key) => { if (context.module === moduleName) { - serverRestrictions[key] = servers + if (!(key in serverRestrictions)) serverRestrictions[key] = servers } }) } }packages/robo/src/default/commands/help.ts (1)
47-56: Micro‑opt: avoid duplicate portal lookups per command.Cache controller/module per iteration to reduce overhead.
- const filteredByAvailability = commands.filter(cmd => { - if (cmd.key && portal.command(cmd.key) && !portal.command(cmd.key).isEnabled) return false - if (cmd.command.__module && portal.module(cmd.command.__module) && !portal.module(cmd.command.__module).isEnabled) return false + const filteredByAvailability = commands.filter((cmd) => { + const pc = cmd.key ? portal.command(cmd.key) : null + if (pc && !pc.isEnabled) return false + const pm = cmd.command.__module ? portal.module(cmd.command.__module) : null + if (pm && !pm.isEnabled) return false if (serverId && cmd.key && !portal.isEnabledForServer(cmd.key, serverId)) return false return true })Apply the same pattern in autocomplete’s filter block.
Also applies to: 84-97
packages/robo/src/types/config.ts (1)
87-90: Document default for keepRegistered.Clarify default behavior in JSDoc for consistency with
Portal.open()logic (defaults to false).- /** When true, disabling a module or component won't unregister commands, events, etc. */ + /** + * When true, disabling a module or component won't unregister commands, events, etc. + * Default: false + */ keepRegistered?: booleanpackages/robo/src/types/global.d.ts (1)
35-36: Fix ESLint no-var for global declaration.Ambient globals require
var, so suppress the rule locally.declare global { - var robo: RoboGlobal + // eslint-disable-next-line no-var + var robo: RoboGlobal }packages/robo/src/core/globals.ts (2)
56-56: Improve type safety by defining a proper type for portal.The
anycast bypasses type checking. Consider defining a proper interface for the portal structure that includesenabledStateandserverRestrictions.Apply this approach to improve type safety:
+interface PortalInternal { + apis: Collection<string, HandlerRecord<Api>> | null; + commands: Collection<string, HandlerRecord<Command>> | null; + context: Collection<string, HandlerRecord<Context>> | null; + events: Collection<string, HandlerRecord<Event>[]> | null; + middleware: HandlerRecord<Middleware>[]; + moduleKeys: Set<string>; + enabledState: PortalEnabledState; + serverRestrictions: Record<string, string[]>; +} -const portalAny = globalThis.robo.portal as any; +const portal = globalThis.robo.portal as PortalInternal;
125-139: Safeguards are redundant after initialization.Lines 127-139 check and initialize
enabledStateandserverRestrictions, butGlobals.init()already initializes these structures at lines 82-89. The safeguards inregisterPortalare defensive but unnecessary if initialization is guaranteed.packages/robo/src/core/handlers.ts (2)
51-57: Consider extracting middleware enablement check logic.The middleware enablement checking pattern is duplicated across autocomplete (lines 51-57), command (lines 138-144), context (lines 283-289), and event handlers (lines 429-435). Consider extracting this into a helper function to reduce duplication and improve maintainability.
Example helper function:
function* getEnabledMiddleware(guildId?: string) { for (const middleware of portal.middleware) { const key = middleware.key || String(portal.middleware.indexOf(middleware)) if (!portal.middlewareController(key).isEnabled) { continue } if (guildId && !portal.isEnabledForServer(key, guildId)) { continue } yield { middleware, key } } }Then use it like:
for (const { middleware } of getEnabledMiddleware(interaction.guildId)) { // execute middleware }
419-419: Improve type safety for guild ID extraction.The
anycast loses type information. Consider using a type guard or more specific typing for the interaction parameter.Apply this diff:
-const interaction = eventData[0] as any -const guildId = interaction?.guildId || interaction?.guild?.id +const interaction = eventData[0] +const guildId = + (interaction && typeof interaction === 'object' && 'guildId' in interaction) + ? (interaction as { guildId?: string; guild?: { id?: string } }).guildId || + (interaction as { guild?: { id?: string } }).guild?.id + : undefinedOr create a helper function:
function extractGuildId(data: unknown): string | undefined { if (!data || typeof data !== 'object') return undefined const obj = data as { guildId?: string; guild?: { id?: string } } return obj.guildId || obj.guild?.id }packages/robo/src/types/index.ts (1)
137-137: Clarify the purpose of empty default export.The empty default export
export default {}is unusual and may cause confusion. If this is intentional to maintain a consistent module structure or for backward compatibility, consider adding a comment explaining why.+// Empty default export for consistent module structure export default {}Alternatively, if not needed:
-export default {}packages/robo/src/core/portal.ts (1)
389-392: Improve type safety with proper portal interface.The
anycasts at lines 389 and 402 bypass type checking. Define a proper type for the portal object to eliminate these unsafe casts.Consider defining an interface at the top of the file:
interface GlobalPortal { serverRestrictions: Record<string, string[]> enabledState: { commands: Record<string, boolean> contexts: Record<string, boolean> events: Record<string, boolean> middleware: Record<string, boolean> } }Then use it:
-const portalAny = globalThis.robo.portal as any +const portal = globalThis.robo.portal as GlobalPortal -if (portalAny && typeof portalAny.serverRestrictions === 'object') { - portalUtils.setServerOnly(portalAny.serverRestrictions, handlerKey, servers); +if (portal?.serverRestrictions) { + portalUtils.setServerOnly(portal.serverRestrictions, handlerKey, servers); }Also applies to: 402-414
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)packages/robo/src/core/globals.ts(4 hunks)packages/robo/src/core/handlers.ts(6 hunks)packages/robo/src/core/portal-utils.ts(1 hunks)packages/robo/src/core/portal.ts(6 hunks)packages/robo/src/default/commands/help.ts(4 hunks)packages/robo/src/types/config.ts(1 hunks)packages/robo/src/types/global.d.ts(1 hunks)packages/robo/src/types/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/robo/src/core/{config,robo,handlers,portal,logger,flashcore,env,state}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Maintain core responsibilities in their designated files: config.ts, robo.ts, handlers.ts, portal.ts, logger.ts, flashcore.ts, env.ts, state.ts
Files:
packages/robo/src/core/handlers.tspackages/robo/src/core/portal.ts
🧠 Learnings (2)
📚 Learning: 2025-10-20T07:38:55.227Z
Learnt from: CR
PR: Wave-Play/robo.js#0
File: AGENTS.md:0-0
Timestamp: 2025-10-20T07:38:55.227Z
Learning: Applies to packages/robo/src/core/{config,robo,handlers,portal,logger,flashcore,env,state}.ts : Maintain core responsibilities in their designated files: config.ts, robo.ts, handlers.ts, portal.ts, logger.ts, flashcore.ts, env.ts, state.ts
Applied to files:
packages/robo/src/types/global.d.tspackages/robo/src/core/portal.ts
📚 Learning: 2025-10-20T07:42:16.211Z
Learnt from: CR
PR: Wave-Play/robo.js#0
File: packages/@robojs/ai/AGENTS.md:0-0
Timestamp: 2025-10-20T07:42:16.211Z
Learning: Applies to packages/robojs/ai/src/index.ts : src/index.ts must re-export the AI singleton, all BaseEngine types, and the PluginOptions interface
Applied to files:
packages/robo/src/types/index.ts
🧬 Code graph analysis (6)
packages/robo/src/types/global.d.ts (5)
packages/robo/src/core/portal.ts (1)
Portal(13-148)packages/robo/src/types/index.ts (4)
HandlerRecord(37-49)Context(23-26)Middleware(60-62)FlashcoreAdapter(28-35)packages/robo/src/types/events.ts (1)
Event(3-5)packages/robo/src/types/config.ts (1)
Config(43-111)packages/robo/src/types/manifest.ts (1)
Manifest(7-25)
packages/robo/src/core/handlers.ts (6)
packages/robo/src/core/portal.ts (2)
command(107-109)middleware(41-43)packages/robo/src/core/robo.ts (1)
portal(46-46)packages/robo/src/core/constants.ts (1)
discordLogger(12-12)packages/robo/src/default/commands/help.ts (1)
interaction(43-82)packages/robo/src/cli/utils/utils.ts (1)
timeout(388-394)packages/robo/src/types/index.ts (1)
HandlerRecord(37-49)
packages/robo/src/core/globals.ts (2)
packages/robo/src/types/index.ts (3)
HandlerRecord(37-49)Context(23-26)Middleware(60-62)packages/robo/src/types/events.ts (1)
Event(3-5)
packages/robo/src/core/portal-utils.ts (2)
packages/robo/src/core/portal.ts (7)
enabledState(53-61)serverRestrictions(49-51)commands(29-31)command(107-109)events(37-39)middleware(41-43)context(33-35)packages/robo/src/core/globals.ts (1)
Globals(35-172)
packages/robo/src/types/index.ts (3)
packages/robo/src/types/commands.ts (2)
CommandContext(43-43)CommandIntegrationType(49-49)packages/robo/src/types/manifest.ts (1)
Manifest(7-25)packages/robo/src/types/api.ts (1)
ApiEntry(28-30)
packages/robo/src/core/portal.ts (3)
packages/robo/src/core/portal-utils.ts (1)
portalUtils(6-144)packages/robo/src/types/index.ts (1)
BaseConfig(123-135)packages/robo/src/core/globals.ts (1)
Globals(35-172)
🪛 ESLint
packages/robo/src/types/global.d.ts
[error] 35-35: Unexpected var, use let or const instead.
(no-var)
packages/robo/src/core/handlers.ts
[error] 419-419: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
packages/robo/src/core/globals.ts
[error] 56-56: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 90-90: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 125-125: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
packages/robo/src/core/portal.ts
[error] 389-389: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 402-402: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (6)
.gitignore (1)
76-78: Add .idea ignore — good housekeeping.No issues. Keeps IDE cruft out of VCS.
packages/robo/src/types/global.d.ts (1)
1-1: ****The review comment misidentifies the nature and scope of the Collection coupling. The suggested replacement with
ReadonlyMap<string, HandlerRecord<T>>is incorrect and would cause runtime failures:
- Collections are instantiated and actively mutated at runtime (portal.ts:329, 420-434) using
.set(), butReadonlyMapis immutable.- The coupling to
discord.jsCollection is at the implementation/runtime level, not just in type definitions—Collections are actual instances stored in Portal, not type-only references.- A viable refactor would require changing runtime code to use
new Map()instead ofnew Collection(), requiring comprehensive changes across portal.ts, globals.ts, global.d.ts, and dependent code.The concern about decoupling from discord.js is valid, but the proposed solution is technically infeasible as stated.
Likely an incorrect or invalid review comment.
packages/robo/src/core/portal-utils.ts (1)
43-56: Gating bykeepRegisteredis properly implemented.All call sites (lines 170–173 in
packages/robo/src/core/portal.ts) correctly gate the unregister functions with the condition!this._getKeepRegistered()before invoking them. The_keepRegisteredvalue is set fromconfig.portal.keepRegistered, ensuring unintended unregisters do not occur when keep-registered mode is enabled.packages/robo/src/core/handlers.ts (1)
88-96: Timeout response type is well-defined.Good improvement to explicitly type the autocomplete timeout response, making it clear what structure is expected.
packages/robo/src/types/index.ts (1)
10-135: Substantial public API expansion looks well-structured.The new type definitions are comprehensive and clearly organized, providing strong typing for the portal enablement system.
packages/robo/src/core/portal.ts (1)
380-393: Server restrictions properly applied during manifest load.The logic correctly extracts
serverOnlyfrom manifest entries and applies restrictions to the portal state. The handling of both array and string formats is appropriate.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @iakzs. * #434 (comment) The following files were modified: * `packages/robo/src/core/handlers.ts` * `packages/robo/src/core/portal.ts`
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/robo/src/core/portal.ts (1)
143-148: Verify Portal instance type before casting.The code casts
globalThis.robo.portaltoPortalwithout verification. Consider adding a type guard or instanceof check to ensure type safety.Apply this diff:
const config = getConfig() if (config?.portal?.keepRegistered) { - if (globalThis.robo?.portal) { + if (globalThis.robo?.portal instanceof Portal) { (globalThis.robo.portal as Portal).setKeepRegistered(true) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/robo/src/core/portal-utils.ts(1 hunks)packages/robo/src/core/portal.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/robo/src/core/portal-utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/robo/src/core/{config,robo,handlers,portal,logger,flashcore,env,state}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Maintain core responsibilities in their designated files: config.ts, robo.ts, handlers.ts, portal.ts, logger.ts, flashcore.ts, env.ts, state.ts
Files:
packages/robo/src/core/portal.ts
🧬 Code graph analysis (1)
packages/robo/src/core/portal.ts (3)
packages/robo/src/core/portal-utils.ts (2)
portalUtils(8-146)ID(4-4)packages/robo/src/core/globals.ts (1)
Globals(35-172)packages/robo/src/types/index.ts (4)
HandlerRecord(37-49)Middleware(60-62)Context(23-26)BaseConfig(123-135)
🪛 ESLint
packages/robo/src/core/portal.ts
[error] 396-396: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 409-409: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (7)
packages/robo/src/core/portal.ts (7)
11-12: Good: Discord.js coupling reduced.The import of
IDtype fromportal-utilsinstead ofSnowflakefromdiscord.jsaddresses the previous critical issue about Discord.js coupling in core. TheCollectionimport from discord.js remains, but this is less problematic than domain-specific types like Snowflake.Based on past review comments.
15-22: LGTM: State tracking fields support granular enablement.The new private fields for per-type enabled state (
_enabledCommands,_enabledEvents, etc.), server restrictions, module controller caching, andkeepRegisteredflag properly support the new granular enablement features.
49-62: LGTM: State getters provide necessary read access.The
serverRestrictionsandenabledStategetters appropriately expose internal state for external consumption while maintaining encapsulation of the private fields.
64-70: LGTM: keepRegistered property accessors.The setter method and getter property provide clean access to the
_keepRegisteredflag.
72-88: LGTM: Module enable/disable methods provide clear API.The
disableModuleandenableModulemethods with clear JSDoc comments provide a straightforward way to control module enablement by delegating toportalUtils.
165-213: Excellent: Handler re-enable logic now implemented.The previous critical issue about handlers not being restored when a module is re-enabled has been properly addressed. Lines 173-212 now iterate through all handler types and restore their enabled flags when
valueistrue, ensuring handlers become functional again after re-enabling a module.The implementation correctly:
- Iterates commands, events, middleware, and contexts
- Checks module membership before re-enabling
- Uses consistent key generation (e.g.,
${eventName}:${index}for events)Based on past review comments.
222-268: LGTM: Consistent controller factory implementations.The controller factories for commands, events, middleware, and contexts follow a consistent pattern with clean delegation to
portalUtils. The implementation quality is good, though the architectural concern about the controller pattern (vs. composable functional API) applies here as well.
| module(moduleName: string) { | ||
| let moduleInstance = this._modules[moduleName] | ||
| if (!moduleInstance) { | ||
| moduleInstance = new Module(moduleName, this._enabledModules) | ||
| moduleInstance = createModuleController( | ||
| moduleName, | ||
| this._enabledModules, | ||
| this._enabledCommands, | ||
| this._enabledEvents, | ||
| this._enabledMiddleware, | ||
| this._enabledContexts, | ||
| this._serverRestrictions, | ||
| () => this._keepRegistered | ||
| ) | ||
| this._modules[moduleName] = moduleInstance | ||
| } | ||
| return moduleInstance | ||
| } | ||
|
|
||
| command(commandName: string) { | ||
| return createCommandController(commandName, this._enabledCommands, this._serverRestrictions) | ||
| } | ||
|
|
||
| event(eventName: string) { | ||
| return createEventController(eventName, this._enabledEvents, this._serverRestrictions) | ||
| } | ||
|
|
||
| middlewareController(middlewareName: string) { | ||
| return createMiddlewareController(middlewareName, this._enabledMiddleware, this._serverRestrictions) | ||
| } | ||
|
|
||
| contextController(contextName: string) { | ||
| return createContextController(contextName, this._enabledContexts, this._serverRestrictions) | ||
| } | ||
|
|
||
| isEnabledForServer(key: string, serverId: ID): boolean { | ||
| // serverId is def real | ||
| return portalUtils.isEnabledForServer(this._serverRestrictions, key, serverId); | ||
| } |
There was a problem hiding this comment.
Factory-based controllers may still not align with maintainer's vision.
While the refactor from classes to factory functions is an improvement, the maintainer explicitly requested "a composable API rather than a controllable/controller-based API" and suggested "an independently exported function/object to interface with."
The current design still follows a controller pattern:
portal.command('foo').setEnabled(false)
portal.module('bar').setServerOnly(['123'])A more functional, composable approach might look like:
// Exported utility functions
setCommandEnabled('foo', false)
setModuleServerOnly('bar', ['123'])
// Or composable
compose(
setEnabled('foo', false),
setServerOnly('bar', ['123'])
)This architectural concern may prevent the PR from being accepted as-is.
Based on maintainer feedback in PR comments.
🤖 Prompt for AI Agents
packages/robo/src/core/portal.ts lines 90-127: the current
factory-based/controller API (portal.command(...).setEnabled(...),
portal.module(...).setServerOnly(...)) still exposes a controller pattern rather
than the maintainer-requested composable/functional API; change the module to
export independent utility functions (e.g., setCommandEnabled(command, enabled),
setModuleServerOnly(module, serverIds), setEnabled(key, value),
setServerOnly(key, serverIds)) and/or small pure action creators that operate on
the underlying state, implement those functions to call the existing helpers
(portalUtils and internal registries) instead of returning controller objects,
and deprecate/remove the controller factory methods
(module/command/event/middlewareController/contextController) so consumers use
the new composable functions or action composers (compose(setEnabled(...),
setServerOnly(...))) to modify configuration.
| if (entry.serverOnly) { | ||
| const servers = Array.isArray(entry.serverOnly) ? entry.serverOnly : [entry.serverOnly] | ||
| const handlerKey = type === 'commands' ? entryKeys.join(' ') : entryKeys[0] | ||
|
|
||
| if (!globalThis.robo) { | ||
| Globals.init() | ||
| } | ||
|
|
||
| // yay | ||
| const portalAny = globalThis.robo.portal as any | ||
| if (portalAny && typeof portalAny.serverRestrictions === 'object') { | ||
| portalUtils.setServerOnly(portalAny.serverRestrictions, handlerKey, servers); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any type with proper typing.
Lines 396 uses any type, which ESLint flags as a violation. Define a proper interface for the expected portal structure instead of using type casting.
As per static analysis hints.
Consider defining an interface:
interface PortalWithRestrictions {
serverRestrictions: Record<string, string[]>
}Then use a type guard:
- const portalAny = globalThis.robo.portal as any
- if (portalAny && typeof portalAny.serverRestrictions === 'object') {
- portalUtils.setServerOnly(portalAny.serverRestrictions, handlerKey, servers);
+ const portal = globalThis.robo.portal
+ if (portal && 'serverRestrictions' in portal && typeof portal.serverRestrictions === 'object') {
+ portalUtils.setServerOnly(portal.serverRestrictions, handlerKey, servers);
}🧰 Tools
🪛 ESLint
[error] 396-396: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
|
Hi |
We've moved these since so I'm just adding your fields to the right place :3
| Globals.init() | ||
| } | ||
|
|
||
| // yay |
| Globals.init() | ||
| } | ||
|
|
||
| const portalAny = globalThis.robo.portal as any |
| plugins?: Plugin[] | ||
| portal?: { | ||
| /** When true, disabling a module or component won't unregister commands, events, etc. */ | ||
| keepRegistered?: boolean |
|
|
||
| const serverId = interaction.guildId | ||
| const filteredByAvailability = commands.filter(cmd => { | ||
| if (cmd.key && portal.command(cmd.key) && !portal.command(cmd.key).isEnabled) return false |
There was a problem hiding this comment.
This is smart! I would've forgotten to cover this use case
2 participants
I've added registering to only an array of servers, disabling modules unregisters their commands, better component disabling and it all should work, however, I'll make the pull request a draft just in case.
Summary by CodeRabbit
New Features
User-visible Behavior
Configuration
portal.keepRegisteredoption to control whether disabling unregisters associated handlers