Skip to content

Conversation

@abcxff
Copy link
Contributor

@abcxff abcxff commented Nov 24, 2025

No description provided.

@vercel
Copy link

vercel bot commented Nov 24, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 24, 2025 8:27pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 24, 2025 8:27pm
rivet-inspector Ignored Ignored Preview Nov 24, 2025 8:27pm
rivet-site Ignored Ignored Preview Nov 24, 2025 8:27pm

Copy link
Contributor Author

abcxff commented Nov 24, 2025

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.

@abcxff abcxff changed the title chore: cleanup rivet-engine tests chore: setup rivet-engine api tests Nov 24, 2025
@abcxff abcxff marked this pull request as ready for review November 24, 2025 20:26
@claude
Copy link

claude bot commented Nov 24, 2025

Code Review: Test Cleanup & Refactoring

This PR successfully reorganizes the rivet-engine test suite, consolidating types and improving test organization. Overall, this is a well-executed refactoring with good improvements to code structure and maintainability.

✅ Strengths

1. Type Consolidation (api-types package)

  • Excellent consolidation of shared types into api-types package
  • New files actors/delete.rs and runners/list_names.rs properly extract duplicate type definitions
  • Consistent use of Serialize and Deserialize derives on all API types (critical for tests)
  • Proper use of pub modifiers on struct fields (e.g., CreateRequest in namespaces.rs)

2. Test Organization

  • Successfully consolidated multiple granular test files into more coherent test suites
  • New test structure with api_* prefix clearly distinguishes API tests from lifecycle tests
  • Eliminated ~3,000 lines of test code while maintaining coverage through better organization
  • Added comprehensive test helpers in common/api/ modules for cleaner test authoring

3. Code Reuse

  • Eliminated duplicate type definitions across api-peer and api-public
  • Both peer and public APIs now share the same request/response types
  • Reduced maintenance burden by having single source of truth for API types

4. Dependency Management

  • Properly added workspace dependencies (rivet-api-types, rivet-types, urlencoding) to engine test package
  • Follows project conventions for workspace dependency management

🐛 Potential Issues

1. Critical: Cursor Pagination Logic Change in actors/list.rs (lines 103-120)

There's a significant change in how pagination works when listing by actor_ids:

// Apply cursor filtering if provided
if let Some(cursor_str) = &query.cursor {
    let cursor_ts: i64 = cursor_str.parse().context("invalid cursor format")?;
    actors.retain(|actor| actor.create_ts < cursor_ts);
}

// Apply limit after cursor filtering
let limit = query.limit.unwrap_or(100);
actors.truncate(limit);

Issues:

  • Previously, the limit was passed to fetch_actors_by_ids() - now it's applied after cursor filtering
  • This changes the semantics: previously you'd get up to N actors, now you filter N actors then apply limit
  • If you request 32 actor IDs with a cursor, you might get 0 results if all are after the cursor
  • This is inconsistent with the key-based path (lines 161-166) which still passes limit to fetch_actors_by_ids

Recommendation: Consider whether this behavioral change is intentional. If so, document it. If not, pass query.limit to fetch_actors_by_ids and apply cursor filtering at the database level for better performance.

2. TypeScript Test Runner Changes (index.ts lines 110-139)

The autoConfigureServerless() function uses a hardcoded endpoint:

const res = await fetch(
    `http://127.0.0.1:6420/runner-configs/${RIVET_RUNNER_NAME}?namespace=${RIVET_NAMESPACE}`,

Issues:

  • Hardcoded 127.0.0.1:6420 doesn't use the RIVET_ENDPOINT env var (which defaults to http://127.0.0.1:6420 but could be different)
  • Inconsistent with the rest of the config which uses RIVET_ENDPOINT

Recommendation: Use RIVET_ENDPOINT instead of hardcoding the URL.

3. Missing Deserialize on DeletePath (api-types/actors/delete.rs line 12)

#[derive(Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct DeletePath {
    pub actor_id: Id,
}

The path struct has Deserialize but path parameters are typically extracted from URL segments, not deserialized from JSON. This works but is unusual.

Minor issue: Not a bug, but worth noting that path parameters don't typically need Deserialize - only Serialize for responses.

📊 Code Quality

Positive:

  • Consistent error handling patterns throughout
  • Proper use of structured logging (though not directly visible in this PR)
  • Good separation of concerns with helper modules
  • Follows Rust idioms and project conventions per CLAUDE.md

Suggestions:

  • Consider adding comments explaining the different pagination paths in actors/list.rs (actor_ids vs key vs fanout)
  • The fanout logic could benefit from inline documentation about why limits are applied differently

🔒 Security Considerations

  • No new security vulnerabilities introduced
  • Proper validation of input parameters (empty checks, length limits on actor_ids)
  • Authentication checks remain in place (ctx.auth().await?)
  • No SQL injection or XSS risks (using type-safe database access)

⚡ Performance Considerations

Positive:

  • Using rivet_pools::reqwest::client() in test helpers (follows CLAUDE.md best practice)
  • Proper use of fanout pattern for cross-datacenter queries
  • Appropriate use of truncation to limit result sizes

Concern:

  • The new cursor filtering logic in actor_ids path loads all actors before filtering, which could be inefficient for large result sets

🧪 Test Coverage

  • Tests appear comprehensive with good coverage of edge cases
  • New test structure makes it easier to understand what's being tested
  • Loss of some granular test files may have removed edge case coverage - recommend reviewing test migration carefully

📝 Additional Notes

  1. Build Impact: The PR adds 3 new dependencies to engine's test suite - minimal impact since they're dev dependencies
  2. Breaking Changes: API behavior changes in pagination could affect external consumers
  3. Documentation: Consider updating API documentation if the pagination behavior change is intentional

Recommendation: ⚠️ Approve with Changes Requested

This is solid refactoring work, but the pagination behavior change in actors/list.rs needs to be addressed or explicitly documented as intentional. The TypeScript endpoint hardcoding should also be fixed.

Action Items:

  1. Review and fix the cursor pagination logic in actor_ids path (critical)
  2. Use RIVET_ENDPOINT in TypeScript autoConfigureServerless (minor)
  3. Verify that test migration didn't lose important edge case coverage (recommended)

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 24, 2025

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3540

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3540

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3540

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3540

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3540

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3540

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3540

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3540

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3540

commit: 4aa8775

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.

2 participants