Repository navigation
chore(backend): fix GuildAutomationExecutionService lint errors - #690
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughGuildAutomationExecutionService refactored to export eight utility functions and restructure type handling with explicit narrowing casts. Imports updated to use new guild automation types, local-only types removed, and a private Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
dependencies
- Added explicit type assertions for Promise.all destructuring (101 errors → 1) - Extracted manifest building logic to separate method to reduce complexity - Wrapped builder parameters in object to meet max-params rule - All 0 remaining eslint errors fixed - All GuildAutomationExecutionService tests passing (28/28) - Full repo lint passes with no errors
…ionService - Cast unknown values to proper types (GuildAutomationRole, GuildAutomationChannel, GuildAutomationParity) - Fix emoji/style type from unknown to string in reaction role mappings - Add proper literal types for module/mode enums in command access grants - Extract typedItem variable to avoid accessing unknown type properties directly - Import missing types from guildAutomation/types module All type checks now pass with no new errors.
10d21ef to
771df2e
Compare
This reverts commit 7dd2b33.
…onExecutionService tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts (1)
1344-1366:⚠️ Potential issue | 🟡 MinorUpdate commandaccess grants test to use valid mode values ('view' or 'manage') instead of 'allow'.
The test uses
mode: 'allow' as any, which masks a type mismatch. The authoritative typeGuildAutomationManifestDocument.commandaccess.grants[*].modeis narrowed to'view' | 'manage'in packages/shared/src/services/guildAutomation/types.ts, and the production code at GuildAutomationExecutionService.ts line 941 casts grant data to that exact union. The invalid'allow'mode should be replaced with a valid literal ('view' or 'manage'), and theas anycast should be removed. There are also similar instances at lines 804, 837, and 1455 in the same test file that use invalid mode values and should be updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts` around lines 1344 - 1366, The test uses an invalid grant mode string ('allow') with an as any cast which masks a type error; update the tests that build manifests (e.g., calls to createMinimalManifest used in the commandaccess grants tests) to use a valid mode literal ('view' or 'manage') instead of 'allow' and remove the "as any" cast so the test matches the authoritative type GuildAutomationManifestDocument.commandaccess.grants[*].mode; make the same replacement for the other similar grant fixtures in the test file that currently use 'allow' with an any-cast so the mocked calls and expectations remain correct when GuildAutomationExecutionService casts grant data.
🧹 Nitpick comments (4)
packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts (1)
1517-1814: Nice added coverage — consider hoisting the repeated mock setup.The four new
captureGuildAutomationStatetests (reaction roles, missing optional mapping fields, command access grants, automessage null normalization) meaningfully exercise the newbuildGuildAutomationManifestpath and its fallback branches. Good addition.One optional cleanup: the block mocking
getManifest/getSettings/getModerationSettings/getWelcomeMessage/getLeaveMessage/listReactionRoleMessages/listExclusiveRoles/listRoleGrantstonull/[]is copy‑pasted in every test (including the pre‑existing ones). Extracting asetDefaultCaptureMocks({ overrides })helper inbeforeEach(or as a test util) would make each test body focus on just what it's verifying. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts` around lines 1517 - 1814, Extract the repeated mock setup into a shared helper (e.g., setDefaultCaptureMocks) and call it from a beforeEach so each test only overrides what it needs; specifically centralize the mocks for guildAutomationService.getManifest, autoModService.getSettings, getModerationSettings, autoMessageService.getWelcomeMessage, autoMessageService.getLeaveMessage, reactionRolesService.listReactionRoleMessages, roleManagementService.listExclusiveRoles, and guildRoleAccessService.listRoleGrants into that helper and allow an optional overrides parameter for tests to replace specific return values.packages/backend/src/services/GuildAutomationExecutionService.ts (3)
814-836: Drop the cast on the already-typeddesired.reactionroles.exclusiveRolesloop.Two observations:
- Line 828 iterates over
desired.reactionroles?.exclusiveRoles, whichGuildAutomationManifestDocumentalready types asArray<{ roleId: string; excludedRoleId: string }>. ThetypedItemcast on line 829 is pure noise and defeats the type safety the manifest already gives you.- For the
existingside (line 814),roleManagementService.listExclusiveRolesreturns PrismaRoleExclusion[]with realroleId/excludedRoleIdcolumns. Casting tounknown[]and then back is a round trip that loses safety rather than gaining anything. Prefer narrowing against the service's actual return type.Also note
unknown & { … }reduces to just{ … }— theunknown &prefix adds no typing constraint.♻️ Suggested diff
- const existing = (await roleManagementService.listExclusiveRoles(guildId)) as unknown[] - - for (const item of existing as unknown[]) { - const typedItem = item as unknown & { roleId: string; excludedRoleId: string } - const key = `${typedItem.roleId}:${typedItem.excludedRoleId}` + const existing = await roleManagementService.listExclusiveRoles(guildId) + + for (const item of existing) { + const key = `${item.roleId}:${item.excludedRoleId}` if (!nextPairs.has(key)) { await roleManagementService.removeExclusiveRole( guildId, - typedItem.roleId, - typedItem.excludedRoleId, + item.roleId, + item.excludedRoleId, ) } } for (const item of desired.reactionroles?.exclusiveRoles ?? []) { - const typedItem = item as unknown & { roleId: string; excludedRoleId: string }; await roleManagementService.setExclusiveRole( guildId, - typedItem.roleId, - typedItem.excludedRoleId, + item.roleId, + item.excludedRoleId, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/GuildAutomationExecutionService.ts` around lines 814 - 836, Remove the unnecessary unknown casts and restore proper types: stop casting desired.reactionroles?.exclusiveRoles items to unknown (remove the typedItem = item as unknown & {...}) and iterate them with their declared type { roleId: string; excludedRoleId: string } so setExclusiveRole(guildId, roleId, excludedRoleId) gets strongly typed inputs; likewise change the existing variable to the real return type of roleManagementService.listExclusiveRoles (e.g., RoleExclusion[]) instead of unknown[] and remove the typedItem cast in the loop that calls removeExclusiveRole so you preserve compile-time type safety around roleId/excludedRoleId while still using nextPairs for membership checks.
561-586: Unnecessaryunknowncasting discards real type information.
autoMessageService.getWelcomeMessage/getLeaveMessagereturn a typed PrismaAutoMessage | nullrecord that already exposesid. Casting first tounknownand then tounknown & { id: string }throws that away — and noteunknown & Tis semantically justT, so the intersection adds nothing. If the lint error was an unsafe-access complaint, narrowing against the real return type (or exporting a type fromautoMessageService) is cleaner than opting out viaunknown.♻️ Suggested simplification
- const existing = ( + const existing = type === 'welcome' ? await autoMessageService.getWelcomeMessage(guildId) : await autoMessageService.getLeaveMessage(guildId) - ) as unknown if (!existing) { ... } - await autoMessageService.updateMessage((existing as unknown & { id: string }).id, { + await autoMessageService.updateMessage(existing.id, { message: payload.message, channelId: payload.channelId, enabled: payload.enabled, })If the lint rule still complains, prefer adding an explicit return-type annotation on the service methods over casting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/GuildAutomationExecutionService.ts` around lines 561 - 586, The code casts the result of autoMessageService.getWelcomeMessage/getLeaveMessage to unknown which discards type info; remove the unnecessary "as unknown" casts and let existing be the real typed return (e.g., AutoMessage | null) so you can safely narrow with if (!existing) and then call (existing.id) when updating; if the linter still complains, add an explicit return type on those service methods (export the AutoMessage type or annotate getWelcomeMessage/getLeaveMessage) rather than using unknown casts.
1032-1053: Duplicate nested-manifest cast; consider a tiny accessor.Lines 881 and 1032 both cast
manifestto essentially the same inline shape ({ manifest?: { version?; parity? } }) to reach into nested fields. That's fine functionally, but it duplicates the assumed shape in two places and makes any future change togetManifest's return fragile. A single narrow helper (or — better — typingguildAutomationService.getManifestproperly) would DRY this up.function getStoredManifest(raw: unknown): { version?: number; parity?: unknown } | undefined { const outer = asObject(raw) const inner = outer ? asObject(outer.manifest) : null return (inner as { version?: number; parity?: unknown } | null) ?? undefined }Minor — not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/GuildAutomationExecutionService.ts` around lines 1032 - 1053, The code duplicates an inline cast of manifest to access nested fields (version/parity) in multiple places; create a single narrow helper (e.g., getStoredManifest(raw: unknown): { version?: number; parity?: unknown } | undefined) or tighten guildAutomationService.getManifest's return type, then replace both ad-hoc casts with calls to that helper when you extract parity (used when calling buildGuildAutomationManifest) and version (used earlier around the other access), so both sites reuse the same typed accessor and remove the duplicated inline cast logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts`:
- Around line 1344-1366: The test uses an invalid grant mode string ('allow')
with an as any cast which masks a type error; update the tests that build
manifests (e.g., calls to createMinimalManifest used in the commandaccess grants
tests) to use a valid mode literal ('view' or 'manage') instead of 'allow' and
remove the "as any" cast so the test matches the authoritative type
GuildAutomationManifestDocument.commandaccess.grants[*].mode; make the same
replacement for the other similar grant fixtures in the test file that currently
use 'allow' with an any-cast so the mocked calls and expectations remain correct
when GuildAutomationExecutionService casts grant data.
---
Nitpick comments:
In `@packages/backend/src/services/GuildAutomationExecutionService.ts`:
- Around line 814-836: Remove the unnecessary unknown casts and restore proper
types: stop casting desired.reactionroles?.exclusiveRoles items to unknown
(remove the typedItem = item as unknown & {...}) and iterate them with their
declared type { roleId: string; excludedRoleId: string } so
setExclusiveRole(guildId, roleId, excludedRoleId) gets strongly typed inputs;
likewise change the existing variable to the real return type of
roleManagementService.listExclusiveRoles (e.g., RoleExclusion[]) instead of
unknown[] and remove the typedItem cast in the loop that calls
removeExclusiveRole so you preserve compile-time type safety around
roleId/excludedRoleId while still using nextPairs for membership checks.
- Around line 561-586: The code casts the result of
autoMessageService.getWelcomeMessage/getLeaveMessage to unknown which discards
type info; remove the unnecessary "as unknown" casts and let existing be the
real typed return (e.g., AutoMessage | null) so you can safely narrow with if
(!existing) and then call (existing.id) when updating; if the linter still
complains, add an explicit return type on those service methods (export the
AutoMessage type or annotate getWelcomeMessage/getLeaveMessage) rather than
using unknown casts.
- Around line 1032-1053: The code duplicates an inline cast of manifest to
access nested fields (version/parity) in multiple places; create a single narrow
helper (e.g., getStoredManifest(raw: unknown): { version?: number; parity?:
unknown } | undefined) or tighten guildAutomationService.getManifest's return
type, then replace both ad-hoc casts with calls to that helper when you extract
parity (used when calling buildGuildAutomationManifest) and version (used
earlier around the other access), so both sites reuse the same typed accessor
and remove the duplicated inline cast logic.
In
`@packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts`:
- Around line 1517-1814: Extract the repeated mock setup into a shared helper
(e.g., setDefaultCaptureMocks) and call it from a beforeEach so each test only
overrides what it needs; specifically centralize the mocks for
guildAutomationService.getManifest, autoModService.getSettings,
getModerationSettings, autoMessageService.getWelcomeMessage,
autoMessageService.getLeaveMessage,
reactionRolesService.listReactionRoleMessages,
roleManagementService.listExclusiveRoles, and
guildRoleAccessService.listRoleGrants into that helper and allow an optional
overrides parameter for tests to replace specific return values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb241853-781a-43c0-ad09-7190f2c9e5f0
📒 Files selected for processing (3)
package.jsonpackages/backend/src/services/GuildAutomationExecutionService.tspackages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts
📜 Review details
🔇 Additional comments (2)
package.json (2)
4-4: LGTM: Improved readability.Replacing the escaped Unicode sequence with a literal em-dash improves readability while maintaining the same output.
86-86: This is a valid monorepo dependency pattern.Adding
eslintto rootdevDependenciesis necessary for the root lint scripts (lines 20-21) to function. The frontend workspace also declares the same version (^10.2.0), which is a standard npm workspaces pattern where:
- Root provides eslint for project-wide linting
- Frontend declares it explicitly for its own environment
- Other workspaces (backend, bot, shared) don't declare it and inherit via hoisting
No redundancy or dependency resolution issues exist here.
- Export utility functions (normalizeName, asObject, toAutoModPayload, toModerationPayload, isExpectedDeleteError, isOnboardingUnavailable, mapChannelType, toDiscordChannelType) - Add comprehensive unit tests for GuildAutomationExecutionError and utility functions - Improve coverage from 86.68% to 90.82%
Resolved package.json conflict by taking main's v2.6.134.
github-actions
Bot
removed
the
dependencies
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/src/services/GuildAutomationExecutionService.ts (1)
152-183:⚠️ Potential issue | 🟠 MajorReject unsupported channel types instead of silently converting them to text.
toDiscordChannelType()falls back to0andmapChannelType()falls back to'GuildText', allowing typos or unsupported manifest channel types to silently create text channels instead of failing. Make text an explicit case and throw on unknown values. Tests at lines 1849 and 1859 currently expect this fallback behavior and will need updating.🐛 Proposed fix
export function mapChannelType(type: number): string { switch (type) { + case 0: + return 'GuildText' case 4: return 'GuildCategory' case 2: return 'GuildVoice' case 5: @@ case 13: return 'GuildStageVoice' default: - return 'GuildText' + throw new GuildAutomationExecutionError( + `Unsupported Discord channel type: ${type}`, + 400, + ) } } export function toDiscordChannelType(type: string): number { switch (type) { + case 'GuildText': + return 0 case 'GuildCategory': return 4 case 'GuildVoice': return 2 case 'GuildAnnouncement': @@ case 'GuildStageVoice': return 13 default: - return 0 + throw new GuildAutomationExecutionError( + `Unsupported manifest channel type: ${type}`, + 400, + ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/GuildAutomationExecutionService.ts` around lines 152 - 183, mapChannelType and toDiscordChannelType currently silently default unknown inputs to 'GuildText' or 0; change them to handle 'GuildText' explicitly and throw on unsupported values instead. In mapChannelType(type: number) add an explicit case for the numeric code that corresponds to 'GuildText' and replace the default branch with a throw(new Error(...)) that includes the provided numeric type; in toDiscordChannelType(type: string) add an explicit case for 'GuildText' and replace the default branch with a throw(new Error(...)) that includes the provided string; reference the functions mapChannelType and toDiscordChannelType when making the changes so tests expecting the old fallback can be updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/backend/src/services/GuildAutomationExecutionService.ts`:
- Around line 152-183: mapChannelType and toDiscordChannelType currently
silently default unknown inputs to 'GuildText' or 0; change them to handle
'GuildText' explicitly and throw on unsupported values instead. In
mapChannelType(type: number) add an explicit case for the numeric code that
corresponds to 'GuildText' and replace the default branch with a throw(new
Error(...)) that includes the provided numeric type; in
toDiscordChannelType(type: string) add an explicit case for 'GuildText' and
replace the default branch with a throw(new Error(...)) that includes the
provided string; reference the functions mapChannelType and toDiscordChannelType
when making the changes so tests expecting the old fallback can be updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae2dd30c-bb02-4cbe-a508-15a9328bd8b8
📒 Files selected for processing (2)
packages/backend/src/services/GuildAutomationExecutionService.tspackages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quality Gates
- GitHub Check: SonarCloud Scan
🔇 Additional comments (2)
packages/backend/src/services/GuildAutomationExecutionService.ts (1)
838-953: Replace no-opunknown & Tassertions with typed inputs or validation.The manifest builder still asserts loosely typed service results into final manifest fields, including command access
module/mode, without runtime validation. Prefer concrete upstream return types or schema parsing before constructing the manifest.#!/bin/bash # Description: Locate remaining no-op unknown intersections and compare command access literals with the shared manifest type. rg -nP --type=ts -C2 'unknown\s*&|as\s+unknown\s*&' packages/backend/src/services/GuildAutomationExecutionService.ts rg -nP --type=ts -C5 'commandaccess|mode:\s*'\''view'\''|mode:\s*'\''manage'\''' packages/shared/src/services/guildAutomation/types.tspackages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts (1)
1528-1825: Good coverage for capture-state edge cases.The added tests cover reaction-role mappings, exclusive-role capture, command access grants, and
null→undefinedautomessage normalization.
|
* chore(backend): eliminate lint errors in GuildAutomationExecutionService - Added explicit type assertions for Promise.all destructuring (101 errors → 1) - Extracted manifest building logic to separate method to reduce complexity - Wrapped builder parameters in object to meet max-params rule - All 0 remaining eslint errors fixed - All GuildAutomationExecutionService tests passing (28/28) - Full repo lint passes with no errors * fix(backend): resolve TypeScript type errors in GuildAutomationExecutionService - Cast unknown values to proper types (GuildAutomationRole, GuildAutomationChannel, GuildAutomationParity) - Fix emoji/style type from unknown to string in reaction role mappings - Add proper literal types for module/mode enums in command access grants - Extract typedItem variable to avoid accessing unknown type properties directly - Import missing types from guildAutomation/types module All type checks now pass with no new errors. * chore: sync package-lock.json after eslint version bump * Revert "chore: sync package-lock.json after eslint version bump" This reverts commit cbcc602afee8dfbbc54686e87d921a8923065716. * test: add reaction roles and command access coverage to GuildAutomationExecutionService tests * test: add utility function tests for GuildAutomationExecutionService - Export utility functions (normalizeName, asObject, toAutoModPayload, toModerationPayload, isExpectedDeleteError, isOnboardingUnavailable, mapChannelType, toDiscordChannelType) - Add comprehensive unit tests for GuildAutomationExecutionError and utility functions - Improve coverage from 86.68% to 90.82%
This branch was successfully deployed
1 participant
Summary
Test plan
Error count before: 101
Error count after: 0
Summary by CodeRabbit
Tests
Refactor