Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

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

Copy link
Member Author

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 10, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 02250ac

@jog1t jog1t force-pushed the 11-09-refactor_rivetkit_extract_websocket_protocol_parsing_to_shared_utility branch from 52895aa to 94422d8 Compare November 12, 2025 17:06
@jog1t jog1t force-pushed the 11-09-chore_rivetkit_add_listing_actors_by_only_name_to_manager_api branch from fddf048 to 0a46ce9 Compare November 12, 2025 17:06
@claude
Copy link

claude bot commented Nov 12, 2025

PR Review: Add listing actors by name to manager API

Summary

This PR adds functionality to list actors by name through the manager API, implementing the listActors method across all manager driver implementations.

Code Quality & Best Practices ✅

Positives:

  • Clean interface extension with proper TypeScript types
  • Consistent implementation across multiple drivers (FileSystem, Remote, Cloudflare Workers, TestInline)
  • Good separation of concerns - API endpoint, driver interface, and implementations are properly layered
  • Proper handling of optional parameters (createTs, includeDestroyed)

Suggestions:

  1. Unused parameter in ListActorsInput: The key field is defined in the interface but never used in any implementation:

    export interface ListActorsInput<E extends Env = any> {
        c?: HonoContext | undefined;
        name: string;
        key?: string;  // ⚠️ Never used in implementations
        includeDestroyed?: boolean;  // ⚠️ Only passed but never actually used
    }
    • The key parameter is passed through in router.ts:367 but not utilized in FileSystem or Remote drivers
    • Either implement filtering by key or remove it from the interface
  2. includeDestroyed parameter is not implemented: While this parameter is passed to listActors, none of the implementations actually filter based on it:

    • FileSystemManagerDriver doesn't check destroyed status
    • RemoteManagerDriver doesn't pass it to the API endpoint
    • Consider adding a TODO comment or implementing the filtering

Potential Issues 🔍

  1. Performance concern in FileSystemManagerDriver (src/drivers/file-system/manager.ts:337-359):

    const itr = this.#state.getActorsIterator({});
    for await (const actor of itr) {
        if (actor.name === name) {
            actors.push({...});
        }
    }
    • This iterates through all actors to find matches by name
    • Could be inefficient if there are many actors
    • Consider adding pagination or warning about potential performance issues
    • Alternatively, maintain an index by actor name in the state
  2. Missing validation in API endpoint: The new listing path doesn't validate or limit the number of results returned. Consider adding:

    • Maximum result limit (similar to the 32 actor ID limit)
    • Pagination support for large actor lists
  3. Type inconsistency: In router.ts:367, key is passed to listActors as a string, but in ListActorsInput it's typed as string | undefined. The router should ensure consistency.

Security Considerations ✅

  • No obvious security issues
  • Authorization context (c: HonoContext) is properly threaded through all calls
  • Input validation is present for the API endpoint

Performance Considerations ⚠️

  1. Sorting overhead in FileSystemManagerDriver: Sorting happens after iteration, which is fine for small datasets but could be optimized:

    actors.sort((a, b) => {
        const aTs = a.createTs ?? 0;
        const bTs = b.createTs ?? 0;
        return bTs - aTs;
    });
    • Consider using a min-heap or maintaining sorted order during insertion for better performance
  2. Remote API call: The new listActorsByName endpoint doesn't include any caching, which means repeated calls will hit the remote API. Consider whether caching would be beneficial.

Test Coverage ❌

Critical Issue: No tests found for the new listActors functionality.

Required tests:

  • Test listing actors by name returns correct results
  • Test empty results when no actors match
  • Test sorting by createTs (most recent first)
  • Test behavior when includeDestroyed is true/false (once implemented)
  • Test API endpoint validation (missing name parameter)
  • Test pagination/limits if implemented

Suggested test file locations:

  • rivetkit-typescript/packages/rivetkit/src/driver-test-suite/tests/list-actors.test.ts
  • API endpoint tests in the router test file

Documentation 📝

  • Consider adding JSDoc comments to the listActors method in the ManagerDriver interface
  • Document the sorting behavior (descending by createTs)
  • Document the incomplete Cloudflare Workers implementation

Minor Issues

  1. Cloudflare Workers incomplete implementation (cloudflare-workers/src/manager-driver.ts:352-358):

    • Returns empty array with a warning
    • Consider adding a TODO comment or issue reference for tracking
  2. Unused import? Check if all new imports are actually used (e.g., ListActorsInput in test files)

Recommendations

Must address before merge:

  1. ✅ Add test coverage for the new functionality
  2. ⚠️ Clarify intended behavior of includeDestroyed parameter - implement or remove
  3. ⚠️ Clarify intended behavior of key parameter in ListActorsInput - implement or remove

Should address:

  1. Add performance limits or pagination for large actor lists
  2. Optimize FileSystemManagerDriver iteration (index by name or add warnings)
  3. Add JSDoc documentation

Nice to have:

  1. Complete Cloudflare Workers implementation or document limitations
  2. Consider caching strategy for remote driver

Overall Assessment

This is a solid implementation of a useful feature with good code organization. The main concerns are:

  • Lack of test coverage (critical)
  • Unused interface parameters that create confusion
  • Potential performance issues with large actor counts

The code follows the existing patterns well and integrates cleanly with the current architecture.

@NathanFlurry NathanFlurry force-pushed the 11-09-refactor_rivetkit_extract_websocket_protocol_parsing_to_shared_utility branch from 94422d8 to 8fdd032 Compare November 13, 2025 01:31
@NathanFlurry NathanFlurry force-pushed the 11-09-chore_rivetkit_add_listing_actors_by_only_name_to_manager_api branch from 0a46ce9 to 02250ac Compare November 13, 2025 01:31
@vercel
Copy link

vercel bot commented Nov 13, 2025

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 13, 2025

Code Review

Thank you for this PR! I've reviewed the changes adding the listActors functionality to the manager API. Here's my feedback:

✅ Positive Aspects

  1. Consistent API Design: The new listActors method follows the existing patterns in the codebase with proper input types and output structures.

  2. Good Type Safety: TypeScript interfaces are properly defined with ListActorsInput and exported through the driver helpers.

  3. Proper Sorting: The FileSystem driver implementation correctly sorts results by createTs in descending order (most recent first).

  4. Proper Router Logic: The router correctly handles the new path where only name is provided without actor_ids or key.

🔍 Issues & Concerns

1. Missing Test Coverage (High Priority)

The PR adds a new API endpoint but doesn't include any tests. According to CLAUDE.md guidelines, tests should be added. Please add:

  • Unit tests for the listActors method in the FileSystem driver
  • Integration tests for the new API endpoint path (GET /actors?name=X)
  • Tests covering edge cases (empty results, multiple actors with same name, sorting validation)

2. Incomplete Cloudflare Workers Implementation (Medium Priority)

cloudflare-workers/src/manager-driver.ts:352-358

The Cloudflare Workers driver returns an empty array with just a warning. This could silently fail in production:

async listActors({ c, name }: ListActorsInput): Promise<ActorOutput[]> {
    logger().warn({
        msg: "listActors not fully implemented for Cloudflare Workers",
        name,
    });
    return [];
}

Recommendation: Either implement this properly using KV list operations or throw an Unsupported error to make it clear this feature isn't available:

throw new Unsupported("listActors");

3. Unused Parameters in ListActorsInput (Low Priority)

manager/driver.ts:96-100

The ListActorsInput interface defines key and includeDestroyed parameters that are never used:

export interface ListActorsInput<E extends Env = any> {
    c?: HonoContext | undefined;
    name: string;
    key?: string;  // Not used anywhere
    includeDestroyed?: boolean;  // Not used anywhere
}

Recommendation: Either implement filtering by these parameters or remove them to avoid confusion. Keeping unused parameters in the interface suggests future functionality but could mislead other developers.

4. Potential Performance Issue (Medium Priority)

drivers/file-system/manager.ts:337-359

The listActors implementation iterates through ALL actors and filters in-memory:

const itr = this.#state.getActorsIterator({});
for await (const actor of itr) {
    if (actor.name === name) {
        actors.push({...});
    }
}

Concerns:

  • No pagination support
  • No limit on results returned
  • Full table scan with O(n) complexity

Recommendation:

  • Add pagination parameters (limit, cursor)
  • Consider adding an index on actor name if this becomes a performance bottleneck
  • Add a maximum result limit to prevent memory issues

5. Missing Optional Timestamp (Low Priority)

manager/driver.ts:104-108

The createTs field is optional in ActorOutput:

export interface ActorOutput {
    actorId: string;
    name: string;
    key: ActorKey;
    createTs?: number;
}

But in router.ts:732, it falls back to Date.now() which could give incorrect timestamps:

create_ts: actor.createTs ?? Date.now(),

Recommendation: Ensure all drivers consistently populate createTs. The fallback to Date.now() means the API could return the current timestamp instead of the actual creation time, which is misleading.

6. Inconsistent Error Handling (Low Priority)

The validation logic in router.ts:354-361 could be clearer. The nested if-else structure makes it hard to follow the different query parameter combinations.

Recommendation: Add code comments documenting the three supported query patterns:

  1. actor_ids only (with optional name for optimization)
  2. name + key for exact match
  3. name only for listing all actors with that name

📝 Minor Suggestions

  1. Code Comment: Add a comment in router.ts:363-370 explaining this is the "list by name" path
  2. Logging: Consider adding debug logging in the router when the list path is taken
  3. Documentation: Update any API documentation to reflect this new query capability

🔒 Security

No security concerns identified. The implementation properly validates inputs and doesn't introduce any obvious vulnerabilities.

🎯 Summary

This is a solid foundation for the listActors functionality, but it needs:

  1. Must have: Test coverage
  2. Should have: Complete Cloudflare implementation or explicit error
  3. Should have: Pagination support for FileSystem driver
  4. Nice to have: Remove unused parameters or implement their functionality

The code quality is good and follows the project conventions. Once the test coverage is added and the performance/pagination concerns are addressed, this will be ready to merge.

Would you like me to help write tests for this functionality?

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