fix: rivetkit build #4147
fix: rivetkit build #4147
Conversation
Code Review: PR #4147 - fix: rivetkit buildOverviewThis PR makes several changes to improve code organization and fix exports in the RivetKit TypeScript package. It also adds a complete Vercel-optimized version of the sandbox example. Positive Changes ✅
Issues & Concerns 🔴1. Breaking API Change Without Clear DocumentationLocation: rivetkit-typescript/packages/rivetkit/src/drivers/file-system/mod.ts The PR changes createFileSystemOrMemoryDriver from accepting two separate parameters to accepting an options object. This is marked as a breaking change but there's no migration guide. The function signature still has persist as a positional parameter, which seems inconsistent with moving to an options pattern. Recommendation: Either provide a migration guide or consider a non-breaking approach using function overloads. 2. Unused waitForNames Method Not Actually RemovedLocation: rivetkit-typescript/packages/rivetkit/src/actor/instance/queue-manager.ts The PR description states: removes the unused waitForNames method from the QueueManager class However, looking at the file (lines 362-413), the waitForNames method is still present and fully implemented. This contradicts the PR description. Is this method actually unused? Should this be removed in a follow-up PR? Or is the PR description incorrect? 3. Potential Import Issues with Drizzle ExportsLocation: rivetkit-typescript/packages/rivetkit/src/db/drizzle/sqlite-core.ts The file has both a wildcard export AND specific named exports from the same module. This is redundant - the wildcard export already exports everything, so the specific exports below don't add value. Recommendation: Either keep only the wildcard export, or remove it and keep only specific exports. 4. Code Duplication Between db/drizzle/mod.ts and db/mod.tsLines 59-82 in db/drizzle/mod.ts and lines 12-34 in db/mod.ts contain duplicate createActorKvStore functions. Recommendation: Extract this to a shared utility function to follow DRY principles. 5. Missing Error Handling in Database ExecuteLocation: rivetkit-typescript/packages/rivetkit/src/db/drizzle/mod.ts:147-160 The executeSqlite function doesn't handle errors during query execution. Recommendation: Add try-catch with proper error logging/rethrowing. 6. Large Generated Example Files in GitThe new sandbox-vercel example includes very large files (index.html: 787 lines, App.tsx: 981 lines, page-data.ts: 1090 lines). Are these hand-written or generated? If generated, should they be in .gitignore instead? Minor Issues
|
Graphite Automations"Test" took an action on this PR • (02/06/26)1 assignee was added to this PR based on Kacper Wojciechowski's automation. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| export { KEYS, makeConnKey } from "@/actor/instance/keys"; | ||
| export type { ActorInstance, AnyActorInstance } from "@/actor/instance/mod"; |
There was a problem hiding this comment.
The export statements need to be reordered according to the project's linting rules. The original order had the type exports for ActorInstance before the KEYS export.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
jog1t
force-pushed
the
02-06-fix_rivetkit_build
branch
from
28afd58 to
cead745
Compare
| import { getRequireFn } from "@/utils/node"; | ||
| import type { Actions, ActorConfig } from "./config"; | ||
| import type { ActionContextOf, ActorContext } from "./contexts"; | ||
| import type { AnyDatabaseProvider } from "./database"; | ||
| import type { ActorInstance } from "./instance/mod"; | ||
| import { DeepMutable } from "@/utils"; | ||
|
|
There was a problem hiding this comment.
Import statements need to be properly sorted according to the project's conventions. Ensure imports are grouped correctly (built-ins first, then external packages, then internal imports) and sorted alphabetically within groups.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
jog1t
force-pushed
the
01-31-feat_dashboard_workflows_ui
branch
from
e17b9e9 to
cb2ef1e
Compare
jog1t
force-pushed
the
02-06-fix_rivetkit_build
branch
from
cead745 to
7cbc7e1
Compare
NathanFlurry
force-pushed
the
01-31-feat_dashboard_workflows_ui
branch
from
cb2ef1e to
a5f2e3e
Compare
NathanFlurry
force-pushed
the
02-06-fix_rivetkit_build
branch
from
7cbc7e1 to
53320bb
Compare
NathanFlurry
changed the base branch from
01-31-feat_dashboard_workflows_ui
to
graphite-base/4147
Merge activity
|
# Description This PR removes the unused `waitForNames` method from the `QueueManager` class and reorganizes imports across several files to improve code organization. It also fixes the Drizzle ORM exports by creating a new `sqlite-core.ts` file that re-exports specific functions from the Drizzle ORM package, rather than exporting everything directly. Additionally, the PR updates the `createFileSystemOrMemoryDriver` function to accept an options object instead of separate parameters, making the API more flexible and consistent. ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? The changes have been tested with the existing test suite to ensure that the removal of the unused method and the reorganization of imports don't break any functionality. ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes
1 participant
Description
This PR removes the unused
waitForNamesmethod from theQueueManagerclass and reorganizes imports across several files to improve code organization. It also fixes the Drizzle ORM exports by creating a newsqlite-core.tsfile that re-exports specific functions from the Drizzle ORM package, rather than exporting everything directly.Additionally, the PR updates the
createFileSystemOrMemoryDriverfunction to accept an options object instead of separate parameters, making the API more flexible and consistent.Type of change
How Has This Been Tested?
The changes have been tested with the existing test suite to ensure that the removal of the unused method and the reorganization of imports don't break any functionality.
Checklist: