-
Notifications
You must be signed in to change notification settings - Fork 131
chore(rivetkit): add listing actors by only name to manager api #3430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 11-09-refactor_rivetkit_extract_websocket_protocol_parsing_to_shared_utility
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
52895aa to
94422d8
Compare
fddf048 to
0a46ce9
Compare
PR Review: Add listing actors by name to manager APISummaryThis PR adds functionality to list actors by name through the manager API, implementing the Code Quality & Best Practices ✅Positives:
Suggestions:
Potential Issues 🔍
Security Considerations ✅
Performance Considerations
|
94422d8 to
8fdd032
Compare
0a46ce9 to
02250ac
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Code ReviewThank you for this PR! I've reviewed the changes adding the ✅ Positive Aspects
🔍 Issues & Concerns1. 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:
2. Incomplete Cloudflare Workers Implementation (Medium Priority)
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 throw new Unsupported("listActors");3. Unused Parameters in ListActorsInput (Low Priority)
The 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)
The const itr = this.#state.getActorsIterator({});
for await (const actor of itr) {
if (actor.name === name) {
actors.push({...});
}
}Concerns:
Recommendation:
5. Missing Optional Timestamp (Low Priority)
The export interface ActorOutput {
actorId: string;
name: string;
key: ActorKey;
createTs?: number;
}But in create_ts: actor.createTs ?? Date.now(),Recommendation: Ensure all drivers consistently populate 6. Inconsistent Error Handling (Low Priority)The validation logic in Recommendation: Add code comments documenting the three supported query patterns:
📝 Minor Suggestions
🔒 SecurityNo security concerns identified. The implementation properly validates inputs and doesn't introduce any obvious vulnerabilities. 🎯 SummaryThis is a solid foundation for the
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? |

No description provided.