Skip to content

chore: update sandbox with drizzle example#4187

Closed
NathanFlurry wants to merge 1 commit intomainfrom
02-12-chore_update_sandbox_with_drizzle_example
Closed

chore: update sandbox with drizzle example#4187
NathanFlurry wants to merge 1 commit intomainfrom
02-12-chore_update_sandbox_with_drizzle_example

Conversation

@NathanFlurry
Copy link
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

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?

Please describe the tests that you ran to verify your changes.

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

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4187 February 12, 2026 21:21 Destroyed
@railway-app
Copy link

railway-app bot commented Feb 12, 2026

🚅 Deployed to the rivet-pr-4187 environment in rivet-frontend

Service Status Web Updated (UTC)
ladle ❌ Build Failed (View Logs) Web Feb 12, 2026 at 9:23 pm
frontend-inspector ❌ Build Failed (View Logs) Web Feb 12, 2026 at 9:23 pm
mcp-hub ✅ Success (View Logs) Web Feb 12, 2026 at 9:23 pm
frontend-cloud ❌ Build Failed (View Logs) Web Feb 12, 2026 at 9:22 pm
website 🕐 Initializing (View Logs) Web Feb 12, 2026 at 9:22 pm

Copy link
Member Author


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.

@NathanFlurry NathanFlurry force-pushed the 02-12-chore_update_sandbox_with_drizzle_example branch from 267633b to e59fe8d Compare February 12, 2026 21:22
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4187 February 12, 2026 21:22 Destroyed
@graphite-app
Copy link
Contributor

graphite-app bot commented Feb 12, 2026

Merge activity

  • Feb 12, 9:22 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Feb 12, 9:23 PM UTC: CI is running for this pull request on a draft pull request (#4188) due to your merge queue CI optimization settings.
  • Feb 12, 9:24 PM UTC: Merged by the Graphite merge queue via draft PR: #4188.

graphite-app bot pushed a commit that referenced this pull request Feb 12, 2026
# Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

## 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?

Please describe the tests that you ran to verify your changes.

## 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
@claude
Copy link

claude bot commented Feb 12, 2026

Pull Request Review

This PR adds Drizzle ORM examples to the sandbox and improves the raw SQLite implementation. The changes are well-structured and demonstrate both raw SQL and Drizzle ORM patterns for working with SQLite in RivetKit actors.

✅ Strengths

  1. Good code organization - The new SQLite examples are cleanly separated into raw and Drizzle variants
  2. Consistent patterns - Both examples follow similar CRUD patterns making them easy to compare
  3. Proper parameterization - The raw SQL implementation correctly uses parameterized queries to prevent SQL injection
  4. Migration setup - Drizzle migrations are properly configured with schema and migration files
  5. Documentation snippets - The frontend page-data.ts includes helpful code snippets for users

🔍 Code Quality Issues

Critical: SQL Injection Vulnerability in toggleTodo (sqlite-raw.ts:30-31)

await c.db.execute(
    "UPDATE todos SET completed = NOT completed WHERE id = ?",
    id,
);

While this uses a parameterized query (which is good), the NOT completed operation is problematic because SQLite uses integers (0/1) for booleans. The NOT operator on integers works, but it's inconsistent with the rest of the codebase which explicitly uses 0 and 1. The Drizzle version correctly handles this:

const newCompleted = existing[0].completed ? 0 : 1;
const result = await c.db.update(todos)
    .set({ completed: newCompleted })

Recommendation: Update the raw SQL version to match this pattern for consistency:

// First get the current state
const rows = await c.db.execute("SELECT completed FROM todos WHERE id = ?", id);
if (!rows[0]) return null;
const newCompleted = rows[0].completed ? 0 : 1;
await c.db.execute(
    "UPDATE todos SET completed = ? WHERE id = ?",
    newCompleted,
    id,
);

Issue: Missing Error Handling in toggleTodo (sqlite-raw.ts:30-36)

The raw SQL toggleTodo doesn't check if the todo exists before updating, unlike the Drizzle version which returns null if not found (mod.ts:24). This inconsistency could cause confusion.

Recommendation: Add existence check:

toggleTodo: async (c, id: number) => {
    const existing = await c.db.execute("SELECT * FROM todos WHERE id = ?", id);
    if (!existing[0]) return null;
    // ... rest of logic
}

Issue: addTodo Missing ID Return (sqlite-raw.ts:18-25)

The raw SQL addTodo doesn't return the generated id, but the Drizzle version does (mod.ts:12-17). This makes the raw version less useful for clients who need to reference the created todo.

Recommendation: Use RETURNING clause or `last_insert_rowid()" to return the full record:

const result = await c.db.execute(
    "INSERT INTO todos (title, created_at) VALUES (?, ?) RETURNING *",
    title,
    createdAt,
);
return result[0];

🎯 Important: Database Query Routing Logic (rivetkit/src/db/mod.ts:67-93)

The new logic distinguishes between parameterized and non-parameterized queries:

if (args.length > 0) {
    // Use db.query() for parameterized queries
    const { rows, columns } = await db.query(query, args);
    // ...
}
// Use db.exec() for non-parameterized queries

This is a smart optimization, but there's a potential issue:

Question: What happens if someone calls execute("INSERT INTO todos VALUES (?, ?)", undefined, undefined)? The args.length > 0 would be true, but the values are undefined. Consider adding validation:

if (args.length > 0 && args.some(arg => arg !== undefined)) {
    // Use parameterized query
}

📝 Minor Issues

  1. Missing newlines at EOF - Several files (migrations.js, JSON files) are missing final newlines. This is flagged in the diff with "No newline at end of file". While not a functional issue, it violates POSIX standards and can cause git diff noise.

  2. Inconsistent ORDER BY - sqliteRawActor.getTodos orders by created_at DESC (newest first) while sqliteDrizzleActor.getTodos uses .orderBy(todos.createdAt) (oldest first, ascending is default). These should match for consistency.

  3. TypeScript allowJs: true - The addition of "allowJs": true to tsconfig.json (line 827) is necessary for importing the migrations.js file, but this weakens type safety. Consider if migrations.js could be .ts instead.

  4. promiseWithResolvers fix - The change in sleep.ts:579 (promiseWithResolvers(() => {})) suggests the function signature changed to require a parameter. This is unrelated to the PR's main purpose but appears to be a necessary fix.

🔒 Security Assessment

Good:

  • Parameterized queries are used throughout
  • No obvious SQL injection vulnerabilities
  • No sensitive data exposure

Note: The onMigrate callback in raw SQL allows arbitrary SQL execution, which is by design but should be well-documented for users to avoid dangerous patterns.

⚡ Performance Considerations

  1. The toggleTodo in Drizzle version does two queries (SELECT then UPDATE). Consider using a single UPDATE with RETURNING clause if the ORM supports it.

  2. The Vite plugin sqlRawPlugin (vite.config.ts:842-854) reads SQL files synchronously with readFileSync. This is fine for development but ensure it's only used during build time.

🧪 Test Coverage

Missing: No tests are included for the new SQLite actors. Consider adding:

  • Unit tests for CRUD operations
  • Tests for edge cases (non-existent IDs, empty strings, etc.)
  • Migration tests

According to CLAUDE.md testing guidelines, tests should be run from the rivetkit-typescript package directory.

📚 Documentation

Good: Frontend snippets show clear examples of both raw and Drizzle usage.

Missing:

  • No README or inline comments explaining when to use raw SQL vs Drizzle
  • No documentation on running db:generate command (added in package.json:529)

✨ Recommendations Summary

  1. Critical: Fix the toggleTodo inconsistency in raw SQL version
  2. High: Add ID return to raw SQL addTodo
  3. Medium: Add error handling for non-existent todos
  4. Medium: Fix ORDER BY inconsistency between raw and Drizzle
  5. Low: Add newlines at end of generated files
  6. Low: Consider adding basic tests
  7. Low: Add brief documentation comments

🎉 Overall Assessment

This is a solid PR that adds valuable examples to the sandbox. The code demonstrates good understanding of SQL injection prevention and follows RivetKit patterns well. The main issues are consistency between the raw and Drizzle implementations and some missing error handling. With the critical issues addressed, this will be a great resource for users learning to work with SQLite in RivetKit actors.

Recommendation: Approve with changes requested for the critical and high-priority issues.

@graphite-app graphite-app bot closed this Feb 12, 2026
@graphite-app graphite-app bot deleted the 02-12-chore_update_sandbox_with_drizzle_example branch February 12, 2026 21:24
@claude
Copy link

claude bot commented Feb 12, 2026

PR Review: SQLite Drizzle Example

This PR adds Drizzle ORM examples to the sandbox, replacing the vanilla SQLite example with a more comprehensive Drizzle-based implementation. Overall, the changes look good with some areas for consideration.


✅ Strengths

  1. Good Example Structure: The new Drizzle examples demonstrate real-world usage patterns with proper schema definitions, migrations, and CRUD operations.

  2. Consistent Implementation: Both sandbox and sandbox-vercel examples are kept in sync, which is important for maintainability.

  3. Improved DB Query Handling: The changes to rivetkit-typescript/packages/rivetkit/src/db/mod.ts properly differentiate between parameterized and non-parameterized queries, which is more secure and flexible.

  4. Documentation Updates: Frontend page-data.ts files are properly updated with new snippets and action templates.


🔍 Code Quality Observations

1. Raw DB Execute Method Enhancement (rivetkit-typescript/packages/rivetkit/src/db/mod.ts:68-94)

The new dual-path approach for query execution is a significant improvement. The code comments could be more descriptive about why this split exists - db.query() is for parameterized queries (better security, prevents SQL injection) while db.exec() is for static SQL statements.

2. SQL Injection Prevention (sqlite-raw.ts)

All raw SQL queries properly use parameterized queries. ✅ This is correctly implemented and prevents SQL injection vulnerabilities.

3. Type Safety in Drizzle Example (sqlite-drizzle/mod.ts:22-30)

The toggleTodo action has good defensive programming with null checking. Minor optimization note: this pattern performs two database queries for a single operation. The current implementation prioritizes correctness over optimization, which is acceptable for an example.

4. Vite Plugin for SQL Files (vite.config.ts:6-16)

The new sqlRawPlugin allows importing .sql files as strings but doesn't appear to be used by the current examples. Consider adding a comment explaining its purpose, removing it if unused, or adding an example that uses it.

5. Timestamp Field Naming (schema.ts:7)

✅ This follows the CLAUDE.md convention: "When storing timestamps, name them *_at with past tense verb."


🐛 Potential Issues

1. Boolean Toggle Logic (sqlite-drizzle/mod.ts:25)

SQLite stores booleans as integers. The current logic is safe, but explicit comparison would be clearer: const newCompleted = existing[0].completed === 1 ? 0 : 1;

2. Missing Error Handling in Raw SQLite Example

The toggleTodo and deleteTodo actions in sqlite-raw.ts return rows[0] which could be undefined if id doesn't exist. Recommend returning rows[0] ?? null to be consistent with the Drizzle example.

3. Generated Files in Version Control

The PR includes auto-generated Drizzle migration files. For examples, committing them makes sense so users can immediately run the example without generating migrations.


🔒 Security

No security concerns identified:

  • All SQL queries use parameterized statements
  • No user input is directly concatenated into SQL strings
  • Drizzle ORM provides additional type safety

📝 Documentation & Testing

Missing Test Coverage: Consider adding basic integration tests for CRUD operations and migration execution.

Documentation: The frontend snippets are well-documented. Consider adding a README explaining when to use raw SQL vs Drizzle and the migration workflow.


📋 Minor Suggestions

  1. The db:generate script is a clever solution for multiple Drizzle configs. Consider documenting this pattern.

  2. allowJs: true was added to tsconfig for migrations.js - this is fine.

  3. The sleep.ts change (promiseWithResolvers(() => {})) appears unrelated to the PR.


✅ Conclusion

This is a solid PR that adds valuable examples to the sandbox. The code quality is good and follows best practices for SQL injection prevention.

Recommendations before merge:

  1. Must: Address the toggleTodo/deleteTodo null return consistency in sqlite-raw.ts
  2. ⚠️ Should: Clarify whether the sqlRawPlugin in vite.config is needed
  3. ⚠️ Should: Consider adding basic tests for the new examples
  4. 💭 Consider: Document the migration generation workflow

Overall: Approve with minor suggestions 🚀

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