Skip to content

fix: rivetkit build#4147

Open
jog1t wants to merge 1 commit into01-31-feat_dashboard_workflows_uifrom
02-06-fix_rivetkit_build
Open

fix: rivetkit build#4147
jog1t wants to merge 1 commit into01-31-feat_dashboard_workflows_uifrom
02-06-fix_rivetkit_build

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Feb 6, 2026

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • 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:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@jog1t jog1t requested a review from NathanFlurry February 6, 2026 19:06
@jog1t jog1t marked this pull request as ready for review February 6, 2026 19:06
Copy link
Contributor Author

jog1t commented Feb 6, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Feb 6, 2026

Code Review - PR #4147: fix: rivetkit build

I've reviewed this PR and have the following feedback:

✅ Positive Aspects

  1. Good refactoring of Drizzle exports: The new sqlite-core.ts file properly isolates Drizzle ORM exports with explicit re-exports. This prevents bloating the bundle with unused exports and improves tree-shaking.

  2. Improved API design: The createFileSystemOrMemoryDriver function now accepts an options object instead of separate parameters, which is more maintainable and extensible.

  3. Import organization: The reorganization of imports across multiple files improves code clarity and reduces coupling.

🔍 Issues and Concerns

1. Unused Method Still Present (queue-manager.ts:363-413)

The PR description claims to remove the waitForNames method, but it's still present in the code (lines 363-413). This appears to be an oversight.

async waitForNames(
    names: string[],
    abortSignal?: AbortSignal,
): Promise<void> {
    // ... implementation still exists
}

Action: If this method is truly unused, it should be removed. If it's being used, the PR description should be updated.

2. Potential Breaking Change Not Clearly Documented

The createFileSystemOrMemoryDriver function signature changed from:

// Before
createFileSystemOrMemoryDriver(persist: boolean, path?: string, useNativeSqlite?: boolean)

// After  
createFileSystemOrMemoryDriver(persist: boolean = true, options?: CreateFileSystemDriverOptionsInput)

While the PR mentions this is a breaking change, any existing code calling this with separate parameters will silently break.

Recommendation:

  • Add migration notes to the PR description
  • Consider a deprecation warning if this is a public API
  • Verify all callsites have been updated

3. Redundant Re-export in sqlite-core.ts:1

export * from "drizzle-orm/sqlite-core";  // Line 1
export {
    blob,
    check,
    // ... specific exports
} from "drizzle-orm/sqlite-core";  // Lines 2-22

The wildcard export on line 1 makes the specific named exports on lines 2-22 redundant. This defeats the purpose of explicit exports.

Action: Remove either the wildcard export (line 1) OR the specific named exports (lines 2-22). I recommend keeping only the specific exports for better tree-shaking.

4. Commented Code in definition.ts:39-44

There's a block of commented-out code that should either be removed or have a clear explanation:

// const requireFn = getRequireFn();
// if (!requireFn) {
//     throw new Error(
//         "ActorDefinition.instantiate requires a Node.js environment",
//     );
// }

Per CLAUDE.md: "Documenting deltas is not important or useful." Either remove this or add a comment explaining why it's temporarily disabled.

5. Import Organization Issues

In definition.ts:2, getRequireFn is imported but only used in one location (line 58). Per CLAUDE.md guidelines, imports should be at the top of files, which is correct here. However, the commented code suggests there may have been a refactoring that wasn't fully completed.

🧪 Testing Concerns

The PR checklist claims "I have added tests that prove my fix is effective or that my feature works," but:

  • No test files were modified or added in this PR
  • The changes include a breaking API change that should have migration tests
  • The Drizzle export refactoring should have integration tests to ensure it still works

Recommendation: Add tests covering:

  1. The new createFileSystemDriver options object API
  2. Drizzle ORM functionality still works with the new exports
  3. Edge cases for the refactored code paths

🎯 Recommendations Summary

High Priority:

  1. Clarify waitForNames removal - Either remove it or update PR description
  2. Fix redundant exports - Remove wildcard export from sqlite-core.ts:1
  3. Add migration guide - Document the breaking change clearly

Medium Priority:
4. ⚠️ Add tests - Ensure new API surface is tested
5. ⚠️ Clean up commented code - Remove or document the commented block

Low Priority:
6. 📝 Code review the import organization - Ensure all import changes were intentional

📊 Overall Assessment

This PR makes valuable improvements to code organization and API design, but has some issues that should be addressed before merging:

  • The main stated objective (removing waitForNames) doesn't appear to be complete
  • The Drizzle exports have a redundancy issue
  • Testing is insufficient for the scope of changes

Verdict: Request changes ⚠️


Reviewed with attention to the CLAUDE.md style guide and RivetKit conventions.

@graphite-app
Copy link
Contributor

graphite-app bot commented Feb 6, 2026

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.

Comment on lines 2 to +3
export { KEYS, makeConnKey } from "@/actor/instance/keys";
export type { ActorInstance, AnyActorInstance } from "@/actor/instance/mod";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@jog1t jog1t force-pushed the 02-06-fix_rivetkit_build branch from 28afd58 to cead745 Compare February 6, 2026 21:13
Comment on lines +2 to 6
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";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant