-
Notifications
You must be signed in to change notification settings - Fork 131
feat(rivetkit): add GET /actors/names
#3411
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
feat(rivetkit): add GET /actors/names
#3411
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: Add
|
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: |
44ab040 to
c5ed5d6
Compare
4f3a2a9 to
42bf4e8
Compare
Code ReviewI've reviewed PR #3411 and found several issues that need to be addressed: Critical Issues1. Unused Query Parameter (Bug)Location: The endpoint accepts a router.openapi(route, async (c) => {
const names = buildActorNames(registryConfig);
return c.json<ActorsListNamesResponse>({
names,
});
});Expected behavior: The handler should either:
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 2. Empty Metadata (Design Issue)Location: The export function buildActorNames(
registryConfig: RegistryConfig,
): Record<string, { metadata: Record<string, any> }> {
return Object.fromEntries(
Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }]),
);
}Questions:
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 Quality3. Schema DefinitionThe 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 StructureThe 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. TestingMissing test coverage: There are no tests included for this new endpoint. Recommendation: Add tests covering:
SummaryMust fix before merge:
Should address:
The overall structure and approach are sound, but the unused parameter is a blocking issue that needs resolution before this can be merged. |
Code ReviewThank you for the contribution! I've reviewed the PR and have the following feedback: Issues Found1. Unused Query Parameter - WarningThe 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:
2. Empty Metadata - InfoThe 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
Minor Observations
Test Coverage RecommendationIf you'd like to add tests, consider testing:
SummaryThe implementation is clean and follows project conventions well. The main concern is the unused Priority: Address the unused |
c5ed5d6 to
819085b
Compare
42bf4e8 to
da963b1
Compare
PR Review: Add
|
PR Review: Add
|
9997c92 to
9624b78
Compare
85e2f9c to
8f15eea
Compare
Code Review for PR #3411: feat(rivetkit): add
|
Code ReviewThank you for adding the ✅ Positives
🔍 Issues & Concerns1. Unused query parameterThe route accepts a request: {
query: z.object({
namespace: z.string(),
}),
},The handler doesn't reference
Recommendation: Either implement namespace filtering or remove the parameter. 2. Empty metadata objectsLooking at Object.keys(registryConfig.use).map((name) => [name, { metadata: {} }])Questions:
Recommendation: Add a comment explaining the purpose of 3. No access control or validationUnlike
Security consideration: Verify this aligns with your security model. If actor names should be protected, add appropriate authorization. 4. Missing testsI couldn't find any tests covering this new endpoint. Recommendation: Add tests to verify:
📝 Minor Observations
🎯 SummaryThis is a clean, small PR that follows existing patterns. The main concerns are:
Action Items
Let me know if you'd like me to elaborate on any of these points! |
9624b78 to
9b70310
Compare
9b70310 to
83a610d
Compare
8f15eea to
5418633
Compare
5418633 to
edd8a45
Compare
Code ReviewThank you for this PR! I've reviewed the changes and have the following feedback: Issues Found1. 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
Suggestions
SummaryThe 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 |
Code Review: feat(rivetkit): add
|
Merge activity
|

No description provided.