Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 7, 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 13, 2025 8:55pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 13, 2025 8:55pm
rivet-inspector Ignored Ignored Preview Nov 13, 2025 8:55pm
rivet-site Ignored Ignored Preview Nov 13, 2025 8:55pm

Copy link
Member Author

NathanFlurry commented Nov 7, 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.

@claude
Copy link

claude bot commented Nov 7, 2025

PR Review: Add GET /actors/names endpoint

Summary

This PR adds a new endpoint to retrieve actor names with metadata. The implementation is clean and follows existing patterns in the codebase.

Code Quality ✅

Positives:

  • Clean implementation following existing patterns in the codebase
  • Proper use of Zod schemas for type safety
  • Consistent with OpenAPI route definitions
  • Schema definitions properly structured with ActorNameSchema and ActorsListNamesResponseSchema

Issues & Concerns

1. Unused Query Parameter ⚠️

Location: rivetkit-typescript/packages/rivetkit/src/manager/router.ts:384-386

The endpoint defines a required namespace query parameter but never uses it:

query: z.object({
    namespace: z.string(),
}),

However, in the handler (line 392), the parameter is never extracted or used. The function simply calls buildActorNames(registryConfig) without any namespace filtering.

Recommendation:

  • If namespace filtering is intended for future use, document this with a TODO comment
  • If not needed, remove the parameter from the schema to avoid confusion
  • If it should be used, implement the filtering logic

2. Empty Metadata 🤔

Location: rivetkit-typescript/packages/rivetkit/src/registry/config.ts:38

The buildActorNames function currently returns empty metadata for all actors:

Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }])

Questions:

  • Is this a placeholder for future metadata population?
  • Should metadata be extracted from the actor definitions?
  • If metadata is always empty, is the field necessary?

Recommendation:

  • Add a comment explaining the purpose of the metadata field
  • Consider if any useful metadata should be populated from actor definitions

3. Missing Documentation 📝

Recommendation:

  • Add JSDoc comments to explain what this endpoint returns and its use case
  • Document the expected metadata structure in ActorNameSchema

Example:

/**
 * Retrieves all registered actor names with their metadata.
 * Used by clients to discover available actor types in the registry.
 */
export const ActorsListNamesResponseSchema = z.object({
    names: z.record(z.string(), ActorNameSchema),
});

Test Coverage ⚠️

Issue: No tests included for the new endpoint.

Recommendation: Consider adding integration tests to verify:

  • Endpoint returns all registered actor names
  • Response schema is correct
  • Handles empty registry gracefully

Performance Considerations ✅

No performance concerns. The buildActorNames function is lightweight and only iterates over the actor registry keys.

Security Considerations ✅

No security concerns identified. The endpoint only exposes actor names (which are already public in the registry) and doesn't leak sensitive information.

Overall Assessment

This is a solid implementation that follows the existing codebase patterns. The main concerns are:

  1. The unused namespace query parameter
  2. Unclear purpose of the empty metadata field
  3. Missing tests and documentation

Recommendation: Address the unused parameter issue before merging. The other items are nice-to-haves but not blockers.


🤖 Generated with Claude Code

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 7, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: edd8a45

@claude
Copy link

claude bot commented Nov 8, 2025

Code Review

I've reviewed PR #3411 and found several issues that need to be addressed:

Critical Issues

1. Unused Query Parameter (Bug)

Location: rivetkit-typescript/packages/rivetkit/src/manager/router.ts:391

The endpoint accepts a namespace query parameter but never uses it:

router.openapi(route, async (c) => {
    const names = buildActorNames(registryConfig);
    return c.json<ActorsListNamesResponse>({
        names,
    });
});

Expected behavior: The handler should either:

  • Use the namespace parameter to filter/scope the results
  • Remove the parameter from the schema if it's not needed

Current behavior: The parameter is validated but ignored, which could confuse API consumers who expect namespace-based filtering.

Recommendation: Either retrieve and use the namespace parameter with c.req.valid("query"), or remove it from the query schema if namespacing isn't needed for this endpoint.

2. Empty Metadata (Design Issue)

Location: rivetkit-typescript/packages/rivetkit/src/registry/config.ts:38

The buildActorNames function returns empty metadata objects for all actors:

export function buildActorNames(
    registryConfig: RegistryConfig,
): Record<string, { metadata: Record<string, any> }> {
    return Object.fromEntries(
        Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }]),
    );
}

Questions:

  • Is this a placeholder for future functionality?
  • Should the metadata be populated from the actor definitions in registryConfig.use?
  • What is the intended purpose of this metadata field?

Recommendation: If metadata is meant to be populated, extract it from the actor definitions. If it's a placeholder, add a TODO comment explaining future plans.

Code Quality

3. Schema Definition

The ActorNameSchema definition looks reasonable:

export const ActorNameSchema = z.object({
    metadata: z.record(z.string(), z.unknown()),
});

However, given that metadata is always empty in the current implementation, consider documenting what this metadata will eventually contain.

4. Response Schema Structure

The response schema is well-structured:

export const ActorsListNamesResponseSchema = z.object({
    names: z.record(z.string(), ActorNameSchema),
});

This provides a good foundation for the API shape.

Testing

Missing test coverage: There are no tests included for this new endpoint.

Recommendation: Add tests covering:

  • Successful retrieval of actor names
  • Proper handling of the namespace parameter (once the bug is fixed)
  • Response schema validation
  • Edge cases (empty actor registry, etc.)

Summary

Must fix before merge:

  1. Either implement namespace parameter usage or remove it from the schema
  2. Clarify the purpose of empty metadata and either populate it or document why it's empty

Should address:

  1. Add test coverage for the new endpoint
  2. Document the intended use case for the metadata field

The overall structure and approach are sound, but the unused parameter is a blocking issue that needs resolution before this can be merged.

@claude
Copy link

claude bot commented Nov 8, 2025

Code Review

Thank you for the contribution! I've reviewed the PR and have the following feedback:

Issues Found

1. Unused Query Parameter - Warning

The namespace query parameter is defined but never used in the endpoint implementation:

query: z.object({
    namespace: z.string(),
}),

However, in the route handler:

router.openapi(route, async (c) => {
    const names = buildActorNames(registryConfig);  // namespace not used
    return c.json<ActorsListNamesResponse>({
        names,
    });
});

Recommendation: Either:

  • Remove the namespace parameter if it's not needed
  • Implement namespace filtering if it's intended functionality
  • Add validation to extract and use it: const { namespace } = c.req.valid("query");

2. Empty Metadata - Info

The buildActorNames function in registry/config.ts:34 always returns empty metadata objects:

return Object.fromEntries(
    Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }]),
);

Question: Is this intentional? If the metadata will be populated later, consider adding a TODO comment. If not, the schema could be simplified.

Positive Aspects

  1. Consistent API Design - Follows the existing pattern used in other endpoints
  2. Type Safety - Proper use of Zod schemas for request/response validation
  3. OpenAPI Integration - Correctly uses createRoute and buildOpenApiResponses
  4. Code Reuse - Appropriately reuses the buildActorNames utility function

Minor Observations

  1. No Tests - Consider adding tests for this endpoint (though I understand this may be added separately)
  2. Documentation - The endpoint purpose is clear from the code, but API documentation would help consumers understand when to use /actors/names vs /metadata (which also returns actor names)

Test Coverage Recommendation

If you'd like to add tests, consider testing:

  • Successful response with actor names
  • Response schema validation
  • Namespace parameter handling (once clarified)

Summary

The implementation is clean and follows project conventions well. The main concern is the unused namespace parameter which should either be implemented or removed before merging.

Priority: Address the unused namespace parameter before merging.

@claude
Copy link

claude bot commented Nov 9, 2025

PR Review: Add GET /actors/names endpoint

Thank you for adding this endpoint! I've reviewed the changes and found several issues that need to be addressed before merging.

Critical Issues

1. Missing Pagination Support

The Rust implementation in engine/packages/api-types/src/actors/list_names.rs shows this endpoint supports pagination with limit and cursor query parameters, and returns a pagination field in the response:

pub struct ListNamesQuery {
    pub namespace: String,
    pub limit: Option<usize>,
    pub cursor: Option<String>,
}

pub struct ListNamesResponse {
    pub names: HashMap<String, ActorName>,
    pub pagination: Pagination,
}

However, the TypeScript implementation is missing these:

In manager-api/actors.ts:

  • Missing limit and cursor in the query schema
  • Missing pagination field in ActorsListNamesResponseSchema

In manager/router.ts:

  • The route doesn't accept limit or cursor parameters
  • The response doesn't include pagination metadata

2. Namespace Parameter Ignored

The endpoint accepts a namespace query parameter (line 380 in router.ts) but never uses it. The buildActorNames function is called with just registryConfig and doesn't filter by namespace. This is inconsistent with how other actor endpoints work and could be a security issue if actors from different namespaces shouldn't be visible to each other.

Location: rivetkit-typescript/packages/rivetkit/src/manager/router.ts:388

// namespace query param is parsed but never used
const names = buildActorNames(registryConfig);

3. Incorrect Implementation Logic

Looking at the Rust implementation in engine/packages/api-public/src/actors/list_names.rs:40-76, this endpoint should:

  1. Validate the namespace exists
  2. Fanout to all datacenters to get actor names
  3. Deduplicate and sort results
  4. Apply pagination

The TypeScript implementation just returns static actor type names from the registry config, which doesn't match the expected behavior. It should return the actual actor instances (by name) that exist in the namespace, not the actor type definitions.

Code Quality Issues

4. Inconsistent Schema Definition

The ActorNameSchema only contains metadata:

export const ActorNameSchema = z.object({
    metadata: z.record(z.string(), z.unknown()),
});

This matches the Rust ActorName struct, but it's unclear what the metadata should contain or how it's populated since buildActorNames just returns empty metadata objects.

Location: rivetkit-typescript/packages/rivetkit/src/manager-api/actors.ts:18-20

Recommendations

  1. Add pagination support:

    • Add limit and cursor to the query schema
    • Add pagination field to the response schema
    • Implement pagination logic in the route handler
  2. Use the namespace parameter:

    • Pass namespace to whatever data source provides the actor names
    • Add validation to ensure the namespace exists
    • Return appropriate error if namespace not found
  3. Fix the implementation:

    • This should return actual actor instances in the namespace, not static type definitions
    • Consider whether this endpoint needs to interact with the driver to get actual actor data
  4. Add tests:

    • Given the complexity of pagination and multi-datacenter behavior, tests would be valuable
    • See engine/packages/engine/tests/actors_list_names.rs for examples of what should be tested

Questions for Clarification

  1. Is the manager router implementation supposed to mirror the engine's API exactly, or is it a simplified version for local development?
  2. If it's meant to be simplified, should the pagination support still be included for API consistency?
  3. Should buildActorNames be updated to accept a namespace parameter, or should this endpoint get the data differently?

Related Files

  • Rust implementation: engine/packages/api-public/src/actors/list_names.rs
  • Type definitions: engine/packages/api-types/src/actors/list_names.rs
  • Rust types: engine/packages/types/src/actors.rs:44-47
  • Tests: engine/packages/engine/tests/actors_list_names.rs

Once these issues are addressed, particularly the pagination support and namespace handling, this will be ready to merge. Let me know if you need any clarification on the feedback!

@claude
Copy link

claude bot commented Nov 9, 2025

PR Review: Add GET /actors/names endpoint

Thank you for the contribution! I've reviewed the changes and have the following feedback:

Critical Issues

1. Unused namespace query parameter (router.ts:381)

The endpoint defines a required namespace query parameter but never uses it in the handler:

// Query parameter is defined...
query: z.object({
    namespace: z.string(),
}),

// ...but not extracted or used in the handler
router.openapi(route, async (c) => {
    const names = buildActorNames(registryConfig);
    return c.json<ActorsListNamesResponse>({
        names,
    });
});

Recommendation: Either:

  • Remove the namespace parameter if it's not needed for the current implementation
  • Extract and use it: const { namespace } = c.req.valid("query"); and pass it to filter or scope the results
  • If it's for future use, document this with a TODO comment

Code Quality Observations

2. Metadata field is always empty (registry/config.ts:38)

The buildActorNames function always returns empty metadata objects:

Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }])

Questions:

  • Is this placeholder implementation intentional?
  • Should metadata be populated from the actor definitions?
  • Consider adding a comment if this is intended for future enhancement

3. Missing test coverage

I couldn't find any tests for the new endpoint.

Recommendation: Add tests covering:

  • Successful retrieval of actor names
  • Response structure validation
  • The namespace parameter behavior (once clarified)

Minor Observations

4. API consistency

The new endpoint follows the existing pattern well (similar to GET /actors). The schema definitions and OpenAPI integration look correct.

5. TypeScript types

The Zod schemas and TypeScript type exports are properly defined:

  • ActorNameSchema
  • ActorsListNamesResponseSchema
  • Type exports ✓

Summary

The implementation is mostly solid and follows the project's patterns. The main blocker is clarifying the purpose of the unused namespace parameter. Once that's addressed and tests are added, this should be good to merge.

Please let me know if you need clarification on any of these points!

@NathanFlurry NathanFlurry force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 9997c92 to 9624b78 Compare November 13, 2025 02:47
@NathanFlurry NathanFlurry force-pushed the 11-07-feat_rivetkit_add_get__actors_names_ branch from 85e2f9c to 8f15eea Compare November 13, 2025 02:47
@claude
Copy link

claude bot commented Nov 13, 2025

Code Review for PR #3411: feat(rivetkit): add GET /actors/names

Summary

This PR adds a new endpoint GET /actors/names to expose the actor names available in the registry. The implementation follows existing patterns in the codebase and is well-structured.

✅ Strengths

  1. Consistent API Design: The endpoint follows the established OpenAPI patterns used throughout the router
  2. Type Safety: Proper use of Zod schemas for request/response validation
  3. Code Reuse: Leverages the existing buildActorNames utility function
  4. Clear Documentation: MARK comments help with code navigation

🔍 Issues & Concerns

1. Unused Query Parameter (Medium Priority)

Location: rivetkit-typescript/packages/rivetkit/src/manager/router.ts:380-382

The endpoint accepts a namespace query parameter but never uses it:

query: z.object({
    namespace: z.string(),
}),

The handler doesn't extract or validate this parameter at all. This creates a confusing API contract.

Recommendation:

  • If namespace filtering is needed, implement it by filtering registryConfig.use based on the namespace parameter
  • If not needed, remove the namespace parameter from the schema
  • If this is for future use, add a TODO comment explaining the intent

2. Empty Metadata (Low Priority)

Location: rivetkit-typescript/packages/rivetkit/src/registry/config.ts:38

The buildActorNames function currently returns empty metadata objects:

return Object.fromEntries(
    Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }]),
);

Questions:

  • What is the intended purpose of the metadata field?
  • Is this a placeholder for future functionality?
  • Should actor definitions contribute their own metadata?

Recommendation: Add comments explaining the purpose or add a TODO if this is incomplete.

3. Missing Test Coverage (Medium Priority)

No tests were added for this new endpoint. Given the existing test infrastructure (I can see driver tests in the repo), this endpoint should have test coverage.

Recommendation: Add at least one integration test verifying:

  • The endpoint returns the expected actor names from the registry
  • The response structure matches the schema
  • (If namespace filtering is implemented) namespace filtering works correctly

4. API Documentation (Low Priority)

While the OpenAPI schema provides some documentation, consider:

  • Adding JSDoc comments to the schema types explaining when/why to use this endpoint
  • Documenting the relationship between this endpoint and /metadata (which also returns actorNames)

🤔 Design Questions

Redundancy with /metadata endpoint?

The /metadata endpoint already returns actor names via actorNames: buildActorNames(registryConfig) (see rivetkit-typescript/packages/rivetkit/src/common/router.ts:127).

Questions:

  • What's the intended difference between these endpoints?
  • Is /actors/names meant to replace the actor names in /metadata?
  • Does /actors/names need to support filtering/pagination that /metadata doesn't?

Recommendation: Clarify the use case distinction in the PR description or code comments.

📋 Suggested Changes

  1. Remove or implement the namespace parameter:
// Option A: Remove if not needed
query: z.object({}),

// Option B: Implement if needed
router.openapi(route, async (c) => {
    const { namespace } = c.req.valid("query");
    const names = buildActorNames(registryConfig);
    // TODO: Filter by namespace if needed
    return c.json<ActorsListNamesResponse>({ names });
});
  1. Add a test (suggested location: tests/driver-*.test.ts):
test("GET /actors/names returns registered actor names", async () => {
    const response = await fetch(`${baseUrl}/actors/names?namespace=test`);
    const data = await response.json();
    expect(data.names).toBeDefined();
    expect(typeof data.names).toBe("object");
});
  1. Add JSDoc documentation:
/**
 * Lists all actor names registered in the registry.
 * Use this endpoint to discover what actor types are available
 * before creating or querying specific actors.
 */
export const ActorsListNamesResponseSchema = z.object({
    names: z.record(z.string(), ActorNameSchema),
});

✨ Minor Style Notes

The code follows the project conventions well:

  • ✅ Proper use of MARK comments
  • ✅ Consistent schema naming (Schema suffix)
  • ✅ Type inference with z.infer
  • ✅ Follows existing route patterns

🎯 Verdict

Status: Needs revision before merge

The implementation is clean and follows good patterns, but the unused namespace parameter is confusing and should be addressed. Test coverage would also significantly improve confidence in this change.

Once the namespace parameter question is resolved and basic tests are added, this will be ready to merge.

@claude
Copy link

claude bot commented Nov 13, 2025

Code Review

Thank you for adding the GET /actors/names endpoint! This appears to be a straightforward addition that exposes actor name information via the manager API. Here's my feedback:

✅ Positives

  1. Consistent patterns: The implementation follows the existing API patterns in the router (using createRoute, buildOpenApiResponses, etc.)
  2. Type safety: Proper Zod schemas and TypeScript types are defined
  3. Code reuse: Leverages the existing buildActorNames utility function

🔍 Issues & Concerns

1. Unused query parameter

The route accepts a namespace query parameter but never uses it:

request: {
    query: z.object({
        namespace: z.string(),
    }),
},

The handler doesn't reference namespace at all. This could be:

  • Dead code - Remove it if not needed
  • Missing implementation - Filter by namespace if that's the intent
  • Future work - Add a TODO comment

Recommendation: Either implement namespace filtering or remove the parameter.

2. Empty metadata objects

Looking at buildActorNames (src/registry/config.ts:34-40), it always returns empty metadata:

Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }])

Questions:

  • What is the intended purpose of the metadata field?
  • Should it include actor definition metadata (like tags, descriptions, etc.)?
  • Is this a placeholder for future functionality?

Recommendation: Add a comment explaining the purpose of metadata, or populate it with relevant actor definition information if available.

3. No access control or validation

Unlike GET /actors which has validation logic (lines 278-299 in router.ts), this endpoint:

  • Doesn't validate the namespace parameter
  • Has no access control checks
  • Could expose all actor names to any caller

Security consideration: Verify this aligns with your security model. If actor names should be protected, add appropriate authorization.

4. Missing tests

I couldn't find any tests covering this new endpoint.

Recommendation: Add tests to verify:

  • The endpoint returns correct actor names from the registry
  • Response format matches the schema
  • Behavior with the namespace parameter (once clarified)

📝 Minor Observations

  1. OpenAPI documentation: The route would benefit from a description field explaining what this endpoint does and when to use it

  2. Comparison with existing metadata endpoint: The /metadata endpoint (router.ts:212-214) already returns actorNames via handleMetadataRequest. Consider if:

    • This new endpoint serves a different purpose
    • There's potential for consolidation
    • Documentation should explain when to use each

🎯 Summary

This is a clean, small PR that follows existing patterns. The main concerns are:

  1. Clarify/implement the namespace parameter
  2. Explain or populate the metadata field
  3. Add test coverage
  4. Consider security implications

Action Items

  • Remove or implement the unused namespace query parameter
  • Document the purpose of metadata in ActorName
  • Add unit/integration tests
  • Add route description to OpenAPI schema
  • Review if this overlaps with existing /metadata endpoint

Let me know if you'd like me to elaborate on any of these points!

@graphite-app graphite-app bot force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 9624b78 to 9b70310 Compare November 13, 2025 20:51
@graphite-app graphite-app bot force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 9b70310 to 83a610d Compare November 13, 2025 20:51
@graphite-app graphite-app bot force-pushed the 11-07-feat_rivetkit_add_get__actors_names_ branch from 8f15eea to 5418633 Compare November 13, 2025 20:51
@NathanFlurry NathanFlurry force-pushed the 11-07-feat_rivetkit_add_get__actors_names_ branch from 5418633 to edd8a45 Compare November 13, 2025 20:53
@claude
Copy link

claude bot commented Nov 13, 2025

Code Review

Thank you for this PR! I've reviewed the changes and have the following feedback:

Issues Found

1. Unused Query Parameter (Critical)

Location: rivetkit-typescript/packages/rivetkit/src/manager/router.ts:391

The endpoint defines a namespace query parameter but never uses it in the handler. The handler calls buildActorNames(registryConfig) without extracting or using the namespace parameter.

Recommendation: Either extract and use the parameter if namespace-based filtering is needed, remove it from the query schema if it's not required, or add a TODO comment if it's a placeholder for future functionality.

2. Empty Metadata (Design Question)

Location: rivetkit-typescript/packages/rivetkit/src/registry/config.ts:38

The buildActorNames function always returns empty metadata objects. Is this intentional or a placeholder for future functionality?

Positive Aspects

  1. Consistent API Design - Follows the existing pattern used in other endpoints
  2. Type Safety - Proper use of Zod schemas with ActorNameSchema and ActorsListNamesResponseSchema
  3. OpenAPI Integration - Correctly integrated with createRoute and buildOpenApiResponses
  4. Clean Structure - Well-organized code with proper imports and schema definitions

Suggestions

  1. Missing Tests - Consider adding tests to verify successful response, schema validation, and edge cases
  2. Documentation - Consider adding JSDoc comments explaining the purpose and use case

Summary

The implementation is clean and follows project conventions well. The main blocking issue is the unused namespace parameter which should be addressed before merging.


Review generated with Claude Code

@claude
Copy link

claude bot commented Nov 13, 2025

Code Review: feat(rivetkit): add GET /actors/names

Summary

This PR adds a new GET /actors/names endpoint to the RivetKit manager API. The implementation is clean and follows the existing patterns, but there are several issues that need to be addressed.


Critical Issues

1. Namespace query parameter is not used ⚠️

Location: rivetkit-typescript/packages/rivetkit/src/manager/router.ts:387-391

The endpoint accepts a namespace query parameter but completely ignores it:

router.openapi(route, async (c) => {
    const names = buildActorNames(registryConfig);
    return c.json<ActorsListNamesResponse>({
        names,
    });
});

The namespace parameter should be extracted from the query and used (or removed from the schema if not needed). Looking at the Rust backend implementation (engine/packages/api-public/src/actors/list_names.rs:40-76), the namespace is actually used to filter results.

Recommendation: Either:

  • Extract and use the namespace parameter: const { namespace } = c.req.valid("query");
  • Or remove it from the query schema if this manager endpoint intentionally returns all names regardless of namespace

2. Missing pagination support ⚠️

Location: rivetkit-typescript/packages/rivetkit/src/manager-api/actors.ts:70-75

The response schema is missing pagination fields that the backend API provides:

export const ActorsListNamesResponseSchema = z.object({
    names: z.record(z.string(), ActorNameSchema),
});

The Rust backend returns (engine/packages/api-types/src/actors/list_names.rs:18-24):

pub struct ListNamesResponse {
    pub names: HashMap<String, ActorName>,
    pub pagination: Pagination,  // ← Missing in TypeScript schema
}

And the backend query accepts limit and cursor parameters that aren't in the RivetKit schema.

Recommendation: Add pagination support to match the backend API:

export const ActorsListNamesResponseSchema = z.object({
    names: z.record(z.string(), ActorNameSchema),
    pagination: z.object({
        cursor: z.string().optional(),
    }),
});

And update the query schema:

query: z.object({
    namespace: z.string(),
    limit: z.number().optional(),
    cursor: z.string().optional(),
}),

3. Incorrect response structure ⚠️

Location: rivetkit-typescript/packages/rivetkit/src/registry/config.ts:34-40

The buildActorNames function returns empty metadata for all actors, but this doesn't match what the actual backend would return. Looking at the test expectations (engine/packages/engine/tests/actors_list_names.rs), the response structure appears to be different - tests expect an array of names rather than a record.

The test at line 47 shows:

let returned_names = body["names"].as_array().expect("Expected names array");

But the TypeScript schema defines it as a record. This is a discrepancy between the test expectations and the actual API types.

Recommendation: Verify the actual backend response format and ensure the TypeScript schema matches. The Rust types suggest it should be a HashMap/record, but the tests suggest an array.


Code Quality Issues

4. Inconsistent import pattern

Location: rivetkit-typescript/packages/rivetkit/src/manager/router.ts:56

The import mixes type and function imports:

import { buildActorNames, type RegistryConfig } from "@/registry/config";

While this is valid, it's inconsistent with the style on line 55:

import type { DriverConfig, RunnerConfig } from "@/registry/run-config";

Recommendation: Keep imports consistent. Either use separate import statements or use the same pattern throughout.


5. Missing JSDoc comments

Location: rivetkit-typescript/packages/rivetkit/src/manager-api/actors.ts:14-16

The new ActorNameSchema lacks documentation explaining what it represents and why metadata is the only field.

Recommendation: Add JSDoc comments:

/**
 * Represents an actor name with associated metadata.
 * Used in the list names endpoint to provide actor name information.
 */
export const ActorNameSchema = z.object({
    metadata: z.record(z.string(), z.unknown()),
});

Test Coverage

6. No tests for the new endpoint ⚠️

The PR adds a new API endpoint but includes no tests. The Rust backend has comprehensive tests for this endpoint in engine/packages/engine/tests/actors_list_names.rs covering:

  • Basic listing
  • Pagination
  • Empty namespaces
  • Error cases (non-existent namespace, invalid cursor)
  • Cross-datacenter fanout
  • Deduplication
  • Alphabetical sorting

Recommendation: Add TypeScript tests for the RivetKit manager endpoint, or at minimum, verify the endpoint works manually and document the testing approach.


Security Considerations

No obvious security issues detected. The endpoint follows the same patterns as other manager API endpoints.


Performance Considerations

The current implementation calls buildActorNames(registryConfig) on every request, which iterates over all actors in the registry config. For a small number of actors this is fine, but consider:

  • Caching the result if the registry config is static
  • Whether this scales appropriately with the expected number of actors

Summary of Required Changes

  1. Must fix: Use or remove the namespace query parameter
  2. Must fix: Add pagination support to match backend API
  3. Should fix: Verify and correct the response structure (record vs array)
  4. 📝 Nice to have: Add tests for the new endpoint
  5. 📝 Nice to have: Add documentation comments
  6. 📝 Nice to have: Consider performance implications for large actor sets

Overall, the implementation is clean and follows good patterns, but needs alignment with the backend API contract before merging.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 13, 2025

Merge activity

  • Nov 13, 9:15 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 13, 9:15 PM UTC: CI is running for this pull request on a draft pull request (#3465) due to your merge queue CI optimization settings.
  • Nov 13, 9:16 PM UTC: Merged by the Graphite merge queue via draft PR: #3465.

graphite-app bot pushed a commit that referenced this pull request Nov 13, 2025
@graphite-app graphite-app bot closed this Nov 13, 2025
@graphite-app graphite-app bot deleted the 11-07-feat_rivetkit_add_get__actors_names_ branch November 13, 2025 21:16
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.

3 participants