Expand how much of the codebase is using Effect; fix excess error emission#270
Expand how much of the codebase is using Effect; fix excess error emission#270
Conversation
PR Summary
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the application's Effect-based runtime management to improve database initialization reliability and reduce excess error emissions. The changes consolidate runtime management into a new AppRuntime module and convert more of the startup logic to use Effect patterns.
Changes:
- Created new AppRuntime.ts to manage the Effect-based ManagedRuntime, moving it from Database.ts
- Converted startup logic in server.ts to use Effect.gen for initialization and graceful shutdown
- Changed getMessageStats to return undefined instead of throwing NotFoundError for empty messages
- Converted registerCommand to return an Effect and updated initDiscordBot to be an Effect
- Refactored database integrity check to run as a daemon with 6-hour intervals
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Sets build target to "esnext" for SSR to enable top-level await |
| app/server.ts | Converts startup to Effect-based initialization with graceful shutdown handlers |
| app/AppRuntime.ts | New file managing ManagedRuntime and providing database access functions |
| app/Database.ts | Removes ManagedRuntime, adds checkpointWal function, refactors integrity check to daemon |
| app/discord/gateway.ts | Converts initDiscordBot to Effect and removes shutdown handlers |
| app/discord/deployCommands.server.ts | Converts registerCommand to return Effect |
| app/discord/activityTracker.ts | Adds null checks for getMessageStats returning undefined |
| app/helpers/discord.ts | Changes getMessageStats to return undefined instead of NotFoundError |
| app/helpers/metrics.ts | Reorders imports (no functional change) |
| app/routes/*.tsx | Updates imports from Database to AppRuntime |
| app/models/*.ts | Updates imports to use AppRuntime for runtime functions and Database for types |
| app/discord/*.ts | Updates imports from Database to AppRuntime |
| app/helpers/cohortAnalysis.ts | Updates imports from Database to AppRuntime |
| app/effects/runtime.ts | Updates imports from Database to AppRuntime |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // App layer: database + PostHog + feature flags | ||
| // FeatureFlagServiceLive depends on both DatabaseService and PostHogService | ||
| const AppLayer = Layer.mergeAll(DatabaseLayer); |
There was a problem hiding this comment.
The comment mentions "database + PostHog + feature flags" but the AppLayer only merges DatabaseLayer. If PostHog and feature flags are intended to be part of this layer, they're missing from the implementation. If they're not yet implemented, the comment should be updated to reflect the current state.
app/server.ts
Outdated
| const handleShutdown = (signal: string) => | ||
| runtime | ||
| .runPromise( | ||
| Effect.gen(function* () { | ||
| yield* logEffect("info", "Server", `Received ${signal}`); | ||
| try { | ||
| yield* checkpointWal(); | ||
| yield* logEffect("info", "Server", "Database WAL checkpointed"); | ||
| } catch (e) { | ||
| yield* logEffect("error", "Server", "Error checkpointing WAL", { | ||
| error: String(e), | ||
| }); | ||
| } | ||
| process.exit(0); | ||
| }), | ||
| ) | ||
| .then(() => runtime.dispose().then(() => console.log("ok"))); |
There was a problem hiding this comment.
The shutdownMetrics() function is no longer being called in the new shutdown handler. The old gateway.ts had a call to shutdownMetrics() before shutdownDatabase(), but this has been removed. PostHog events may not be flushed properly during shutdown, potentially losing analytics data. Consider adding a call to shutdownMetrics() in the shutdown handler in server.ts, or ensuring PostHog is properly disposed through the runtime.
Preview environment removedThe preview for this PR has been cleaned up. |
Use SqliteClient.make with Layer.scoped so the connection lifetime is tied to the layer scope (Effect.scoped was closing it immediately). Inline PRAGMA busy_timeout into the layer construction. Convert integrity check to Effect.repeat(Schedule) and checkpointWal to return an Effect. Remove runtime, bridge functions, and shutdown helpers that move to AppRuntime in the next commit. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move ManagedRuntime, layer composition, and bridge functions (db, run, runTakeFirst, runTakeFirstOrThrow, posthogClient) from Database.ts into a dedicated AppRuntime.ts. The AppLayer now includes DatabaseLayer, PostHogServiceLive, and FeatureFlagServiceLive. Update all consumer imports to use #~/AppRuntime. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Wrap the startup sequence in an Effect.gen pipeline: init Discord gateway, fork integrity check as a daemon fiber, register commands, and wire up graceful shutdown (WAL checkpoint + runtime dispose). Convert gateway init and registerCommand to return Effects so they compose into the startup pipeline. Move shutdown handler from gateway to server.ts where the runtime is available. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
vcarl
force-pushed
the
vc-expand-effect-radius
branch
from
6090554 to
95c009e
Compare
1 participant
Swaps out the way the database loads, and shift more responsibility for startup into AppRuntime (an Effect).
This also reworks how the database is initalized, which might help with some of the weirdo oddball failures we've been seeing. I'm using a new creation mechanism, and it demanded I explciitly provide a "Scope" reference. I think this is going to mean that all access relies on the same connection, which has been the main problem so far.