-
Notifications
You must be signed in to change notification settings - Fork 6
Improve DX, fix duplicate mod-log threads, use Effect for some Discord APIs #258
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
base: main
Are you sure you want to change the base?
Conversation
- Create app/effects/discordSdk.ts with Effect-wrapped Discord.js helpers: fetchGuild, fetchChannel, fetchMember, fetchMessage, sendMessage, etc. - Add fetchSettingsEffect to guilds.server.ts for Effect-based settings fetch - Update escalate, report, and resolver code to use new SDK helpers - Consolidate error types in effects/errors.ts (rename discordError -> cause) - Fix missing Layer imports and service layer provision in handlers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Discord fires multiple AUTO_MODERATION_ACTION_EXECUTION events for the same trigger (one per action type: BlockMessage, SendAlertMessage, etc.). When these arrive within milliseconds, both can check for existing threads before either creates one, resulting in duplicate threads. Add time-window deduplication that only processes the first automod event per user/guild within a 1-second window. Subsequent events log at debug level and return early. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add main branch protection to pre-commit hook - Create post-checkout hook to auto-run npm install when deps change - Simplify post-merge to auto-run npm install instead of printing reminder - Remove redundant format check from validate script (pre-commit handles it) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
It's currently hardcoded to use specific channel IDs in Reactiflux, so gate it off for anything else right now
Preview deployedIt may take a few minutes before the service becomes available.
Deployed commit: This preview will be updated on each push and deleted when the PR is closed. |
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.
Pull request overview
This PR improves Discord integration ergonomics and dev experience by introducing Effect-based wrappers for common Discord.js operations, centralizing error types, and adding HMR-aware gateway initialization, while also gating Star Hunter features to a specific guild and tightening Stripe / NotFoundError usage.
Changes:
- Add an Effect-based Discord SDK (
app/effects/discordSdk.ts) and replace ad-hoc Discord.js calls with these helpers across escalation, moderation logging, and reporting flows, standardizingDiscordApiError/NotFoundErroruse. - Introduce a database-backed
fetchSettingsEffectinguilds.server.tsand refactor moderation/escalation code to consume settings via Effect, alongside new HMR helpers (hmrRegistry, scheduler handle) for the Discord gateway and escalation resolver. - Restrict Star Hunter UI/routes to the Reactiflux guild, adjust pre-commit and Git hooks to enforce branch workflow and auto-install on dependency changes, and remove the legacy
app/helpers/errors.tsin favor ofapp/effects/errors.ts.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updates validate script to drop format, focusing on tests, lint, and typecheck for faster DX. |
| app/routes/__auth/sh-user.tsx | Guards the Star Hunter user route to only respond for the Reactiflux guild, returning 404 otherwise. |
| app/routes/__auth/dashboard.tsx | Similarly gates the Star Hunter dashboard loader to the Reactiflux guild, avoiding exposure for other guilds. |
| app/models/userThreads.ts | Switches DiscordApiError to use a cause field and keeps thread/mod-log operations wrapped in Effect.tryPromise for better error tagging. |
| app/models/stripe.server.ts | Migrates NotFoundError to the new Effect-based error type and narrows the “price not found” failure path, but currently passes an empty id when no price is returned. |
| app/models/guilds.server.ts | Imports Effect/DatabaseService and adds fetchSettingsEffect for Effect-based settings access, but the implementation incorrectly treats the full query result as a single row and will throw during JSON parsing. |
| app/helpers/schedule.ts | Introduces ScheduledTaskHandle and makes scheduleTask return timer handles for HMR cleanup, with numeric intervals using setTimeout + setInterval and cron schedules returning undefined. |
| app/helpers/errors.ts | Removes the legacy NotFoundError helper in favor of the tagged NotFoundError in app/effects/errors.ts. |
| app/helpers/discord.ts | Switches NotFoundError imports to the Effect-based version and improves the getMessageStats missing-content error with resource/id fields. |
| app/effects/errors.ts | Normalizes error types: adds NotAuthorizedError near the top, replaces discordError/stripeError with a generic cause, collapses escalation not-found into the generic NotFoundError, and keeps other escalation errors intact. |
| app/effects/discordSdk.ts | Adds Effect-wrapped helpers for fetching guilds/channels/members/users/messages and sending/editing/forwarding messages, plus “safe” forwarding/reply helpers; however, replyAndForwardSafe currently maps errors to null, which degrades logged error detail. |
| app/discord/hmrRegistry.ts | Introduces global HMR state helpers for login/ready flags and scheduled tasks, and a listener registry cleaner that relies on globalThis.__discordListenerRegistry, though nothing currently populates that registry. |
| app/discord/gateway.ts | Refactors gateway init to be HMR-aware (single login, rebindable listeners, sub-module init, scheduler startup) and uses the new HMR helpers, but currently does not actually unregister existing listeners before rebinding. |
| app/discord/escalationResolver.ts | Reworks escalation resolver to run via runEffectExit with DatabaseLayer + EscalationServiceLive, and uses scheduleTask plus registerScheduledTask for periodic runs; only the initial timeout is registered, so the repeating interval is not cleaned up on HMR. |
| app/discord/client.server.ts | Declares all Discord HMR-related globals in one place and keeps the client/login behavior the same otherwise. |
| app/components/DiscordLayout.tsx | Hides the Star Hunter nav item and separator unless the current guild is Reactiflux, keeping the UI aligned with the loader gating. |
| app/commands/report/userLog.ts | Updates DiscordApiError construction to use the new cause field while continuing to use fetchSettings and Effect-based logging of user reports. |
| app/commands/report/modActionLogger.ts | Imports the extensionless modActionLogger path and adds a Set-based dedupe for automod triggers to avoid duplicate mod-log entries within a short time window. |
| app/commands/report/modActionLog.ts | Refactors mod-action logging to fetch settings via fetchSettingsEffect and to use sendMessage/forwardMessageSafe, reducing duplicated Discord.js boilerplate. |
| app/commands/report/constructLog.ts | Switches settings retrieval to fetchSettingsEffect and routes internal errors through DiscordApiError with a cause, improving consistency. |
| app/commands/report/automodLog.ts | Mirrors modActionLog changes for automod logs, using fetchSettingsEffect and the new Discord SDK wrappers. |
| app/commands/escalate/vote.ts | Uses fetchSettingsEffect for moderator/restricted role settings and continues to enforce mod and resolution rules via Effect-based logic. |
| app/commands/escalate/service.ts | Replaces EscalationNotFoundError with a generic NotFoundError for escalations and updates the service interface and implementations accordingly. |
| app/commands/escalate/index.ts | Simplifies this module to just provide getFailure for extracting typed failures from Cause, removing the now-inlined runEscalationEffect helper. |
| app/commands/escalate/handlers.ts | Injects DatabaseLayer/EscalationServiceLive directly into escalations-related effects via Effect.provide, and updates not-found branching to match the new NotFoundError tag. |
| app/commands/escalate/expedite.ts | Uses fetchSettingsEffect for moderator role settings and updates guild fetch errors to use DiscordApiError’s new cause field. |
| app/commands/escalate/escalationResolver.ts | Switches all Discord-fetch operations to the new SDK helpers and fetchSettingsEffect, and routes “update vote message + reply/forward” flows through editMessage and replyAndForwardSafe. |
| app/commands/escalate/escalate.ts | Refactors create/upgrade escalation flows to use fetchSettingsEffect and Discord SDK helpers (fetchGuild, fetchChannel, sendMessage, fetchMessage, editMessage) instead of inline Effect.tryPromise blocks. |
| app/commands/escalate/directActions.ts | Centralizes member fetching via fetchMember and uses fetchSettingsEffect for moderator role lookup, while updating all Discord operations to use DiscordApiError with cause. |
| .husky/pre-commit | Adds a guard that rejects commits directly to main, enforcing a feature-branch workflow before running lint-staged. |
| .husky/post-merge | Simplifies post-merge behavior to automatically run npm install when package-lock.json has changed. |
| .husky/post-checkout | Adds a new hook that runs npm install after branch checkouts when package-lock.json differs between the old and new refs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clean up old listeners and scheduled tasks before rebinding | ||
| removeAllListeners(client); | ||
| clearScheduledTasks(); | ||
|
|
||
| // Bind all listeners (runs on every HMR reload) | ||
| bindListeners(); | ||
|
|
Copilot
AI
Jan 24, 2026
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.
removeAllListeners(client) currently relies on globalThis.__discordListenerRegistry, but nothing in the codebase populates that registry, so this call is effectively a no-op. Because bindListeners() is invoked on every HMR reload without actually removing the previously-registered listeners (including the process.on handlers at the bottom of this function), you will accumulate duplicate event handlers in dev. Either add a helper that wraps client.on/client.once and records listeners into the registry, or use the built-in client.removeAllListeners()/client.removeAllListeners(event) for cleanup before rebinding.
| if (!price) { | ||
| log("error", "Stripe", "Failed to load pricing data"); | ||
| throw e; | ||
| throw new NotFoundError({ resource: "Price", id: priceId }); |
Copilot
AI
Jan 24, 2026
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.
When no price is found, the NotFoundError is constructed with id: priceId, but priceId is still an empty string at this point because it is only set after a price is found. This makes the error less useful for debugging and observability; it would be more informative to populate id with the lookup key (variant) or the actual price.id when available.
| throw new NotFoundError({ resource: "Price", id: priceId }); | |
| throw new NotFoundError({ resource: "Price", id: variant }); |
| Effect.tryPromise({ | ||
| try: async () => { | ||
| const reply = await message.reply({ content }); | ||
| await reply.forward(forwardToChannelId); | ||
| return reply; | ||
| }, | ||
| catch: () => null, | ||
| }).pipe( | ||
| Effect.catchAll((error) => | ||
| logEffect("warn", serviceName, "Could not reply and forward message", { | ||
| error, | ||
| messageId: message.id, | ||
| forwardToChannelId, | ||
| }), | ||
| ), |
Copilot
AI
Jan 24, 2026
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.
In replyAndForwardSafe, the catch handler passed to Effect.tryPromise returns null, so any failure from message.reply or reply.forward is converted into a null error. As a result, the error you log in catchAll will always be null, losing the original exception details that would be useful for debugging. To preserve diagnostics while still treating failures as non-fatal, consider letting tryPromise propagate the original error (or wrapping it in a domain error) and only handling it in catchAll, instead of mapping it to null in the catch handler.
| const result = Object.entries( | ||
| yield* db | ||
| .selectFrom("guilds") | ||
| // @ts-expect-error This is broken because of a migration from knex and | ||
| // old/bad use of jsonb for storing settings. The type is guaranteed here | ||
| // not by the codegen | ||
| .select<DB, "guilds", SettingsRecord>((eb) => | ||
| keys.map((k) => eb.ref("settings", "->").key(k).as(k)), | ||
| ) | ||
| .where("id", "=", guildId), | ||
| ) as [T, string][]; |
Copilot
AI
Jan 24, 2026
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.
fetchSettingsEffect is currently calling Object.entries on the full query result set instead of a single row, which means result will be derived from the array of rows rather than the guild settings object. This will cause JSON.parse(v) to receive non-string values (e.g. the row object itself) and throw at runtime, and callers like fetchSettingsEffect(...).modLog / moderator will never receive correct data. You likely want to mirror the non-effect fetchSettings implementation by first selecting a single row (e.g. destructuring the first element) and then calling Object.entries on that row before parsing the values.
| const result = Object.entries( | |
| yield* db | |
| .selectFrom("guilds") | |
| // @ts-expect-error This is broken because of a migration from knex and | |
| // old/bad use of jsonb for storing settings. The type is guaranteed here | |
| // not by the codegen | |
| .select<DB, "guilds", SettingsRecord>((eb) => | |
| keys.map((k) => eb.ref("settings", "->").key(k).as(k)), | |
| ) | |
| .where("id", "=", guildId), | |
| ) as [T, string][]; | |
| const row = yield* db | |
| .selectFrom("guilds") | |
| // @ts-expect-error This is broken because of a migration from knex and | |
| // old/bad use of jsonb for storing settings. The type is guaranteed here | |
| // not by the codegen | |
| .select<DB, "guilds", SettingsRecord>((eb) => | |
| keys.map((k) => eb.ref("settings", "->").key(k).as(k)), | |
| ) | |
| .where("id", "=", guildId) | |
| .executeTakeFirstOrThrow(); | |
| const result = Object.entries(row) as [T, string][]; |
| // The interval timer is created inside the setTimeout, so we need to | ||
| // register it when it's available. Since clearScheduledTasks clears both | ||
| // timeouts and intervals, the initial timer registration will handle cleanup. |
Copilot
AI
Jan 24, 2026
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.
Only the initialTimer from scheduleTask is being registered for HMR cleanup, but the repeating setInterval created inside the timeout is never tracked. Because clearScheduledTasks iterates over the registered timers, this means the repeating escalation resolver interval will continue running after HMR reloads, leading to duplicate schedulers. To ensure proper cleanup, you should also register the interval handle when it is created (or change the scheduler API to return and register both handles explicitly).
| // The interval timer is created inside the setTimeout, so we need to | |
| // register it when it's available. Since clearScheduledTasks clears both | |
| // timeouts and intervals, the initial timer registration will handle cleanup. | |
| // Also register the repeating interval timer if it is exposed by the scheduler | |
| if ((handle as any).intervalTimer) { | |
| registerScheduledTask((handle as any).intervalTimer); | |
| } |
No description provided.