docs: clean up last few missing pieces of content #3538
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
How to use the Graphite Merge QueueAdd 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. |
More templates
@rivetkit/cloudflare-workers
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
PR Review: Documentation Content CleanupThis PR makes significant improvements to the documentation by cleaning up inconsistencies, adding clarity, and fixing several issues. Overall, this is excellent work that will improve the developer experience. ✅ Strengths
🔍 Issues Found1. Code Example Issues in connections.mdxLine 145-148: The lifecycle sequence incorrectly shows createState under "On Connect". createState is only called during "On Create" (first time). The connection lifecycle should only list: onBeforeConnect, createConnState, and onConnect. Line 28-50: References to undefined functions validateToken() and getUserIdFromToken(). Consider adding comments to indicate these are user-defined. 2. Code Issue in external-sql.mdxLines 66 and 183: Both examples use onCreate with parameter named opts but never use it. Should be input to match the pattern. onStateChange pattern: This hook fires on EVERY state change, so updating requestCount triggers a database write. This could cause excessive writes. Consider adding a performance note about this. 3. Minor Inconsistencieslifecycle.mdx Line 9: Uses numbered lists for "On Create" but not other sections - consider consistency. Duplication: Content about createConnState appears in multiple places (lines 87-109 and 260-267). 📝 Suggestions
🔒 Security✅ Good: Examples properly demonstrate authentication validation 🎯 Performance✅ Connection pooling implemented correctly ✨ Overall AssessmentHigh-quality documentation work that significantly improves clarity. Main issues are minor code example inconsistencies and potential performance pitfalls that should be documented. Recommendation: Approve with minor revisions to fix the technical inaccuracies noted above. Great work! 🚀 |
NathanFlurry
force-pushed
the
11-24-docs_clean_up_last_few_missing_pieces_of_content
branch
from
5d83d9c to
1724b94
Compare
PR Review: Documentation Clean UpSummaryThis PR significantly improves the Rivet Actors documentation by filling in missing content, fixing typos, adding comprehensive examples, and improving overall organization. The changes enhance clarity and provide better guidance for developers working with Rivet Actors. ✅ Strengths1. Excellent Content Additions
2. Code Quality Improvements
3. Proper Conventions
🔍 Issues FoundCritical: Typo in CORS DocumentationFile: - See tracking issue for [configuring CORS per-actor on the gateway](https://github.com/rivet-dev/rivet/issues/3539) that will remove the need to implement origin restrictions in `onBforeRequest`.
+ See tracking issue for [configuring CORS per-actor on the gateway](https://github.com/rivet-dev/rivet/issues/3539) that will remove the need to implement origin restrictions in `onBeforeConnect`.Issue: Minor: Potential Logic Issue in Rate Limiter ExampleFile: The rate limiter example has a potential issue - it never resets the counter: const api = actor({
state: { requestCount: 0, lastReset: Date.now() },
actions: {
makeRequest: (c) => {
c.state.requestCount++;
const limit = 100;
if (c.state.requestCount > limit) {
const resetAt = c.state.lastReset + 60_000; // Reset after 1 minute
throw new UserError("Rate limit exceeded", {
code: "rate_limited",
metadata: {
limit: limit,
resetAt: resetAt,
retryAfter: Math.ceil((resetAt - Date.now()) / 1000)
}
});
}
// Rest of request logic...
}
}
});Issue: The Suggestion: makeRequest: (c) => {
// Reset counter if time window has passed
if (Date.now() - c.state.lastReset > 60_000) {
c.state.requestCount = 0;
c.state.lastReset = Date.now();
}
c.state.requestCount++;
const limit = 100;
if (c.state.requestCount > limit) {
const resetAt = c.state.lastReset + 60_000;
throw new UserError("Rate limit exceeded", {
code: "rate_limited",
metadata: {
limit: limit,
resetAt: resetAt,
retryAfter: Math.ceil((resetAt - Date.now()) / 1000)
}
});
}
}Minor: Inconsistent Example in connections.mdxFile: The createConnState: (c, params: ConnParams): ConnState => {
// Validate authentication token
const authToken = params.authToken;
if (!authToken || !validateToken(authToken)) {
throw new Error("Invalid auth token");
}
// Create connection state
return { userId: getUserIdFromToken(authToken), role: "player" };
},Issue: The functions
💡 Suggestions1. Add Missing Type Exports in ExamplesSome examples reference import { actor, setup } from "rivetkit"; // Add 'setup' to importsThis is inconsistent across files - some have it, some don't. 2. Consider Adding a Security Note to CORSThe CORS documentation could benefit from emphasizing that origin validation is security-critical: <Warning>
Always implement origin restrictions for publicly exposed actors. Without origin validation, any website can connect to your actors and potentially abuse resources or access sensitive data.
</Warning>3. Example Database ObjectsThe design patterns examples reference // Note: 'db' represents your database client (e.g., Prisma, Drizzle, or raw SQL)
// This is just an example - implement according to your database setup📊 Statistics
✅ Test CoverageThis PR is documentation-only with one small code fix (typo correction). The typo fix in Recommendation: Consider adding a quick smoke test to verify the documentation examples compile correctly if you have such infrastructure. 🎯 Final VerdictStatus: ✅ Approve with Minor Fixes Required This is an excellent documentation improvement PR that significantly enhances the developer experience. The issues found are minor and easily fixable:
The documentation is well-structured, comprehensive, and follows proper conventions. Great work! 🎉 Reviewed by Claude Code |
NathanFlurry
force-pushed
the
11-24-docs_clean_up_last_few_missing_pieces_of_content
branch
from
1724b94 to
799cad2
Compare
Merge activity
|
graphite-app
bot
deleted the
11-24-docs_clean_up_last_few_missing_pieces_of_content
branch
No description provided.