Conversation
|
🚅 Deployed to the rivet-pr-3961 environment in rivet-frontend
|
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. |
efa3faa to
009f2c1
Compare
Merge activity
|
Graphite Automations"Test" took an action on this PR • (01/17/26)1 assignee was added to this PR based on Kacper Wojciechowski's automation. |
### TL;DR Added actor count query functionality and improved the onboarding experience with better error handling and deployment URL formatting. ### What changed? - Added a new `actorsCountQueryOptions` function to efficiently query the count of actors - Updated the frontend setup to use the new actor count query instead of fetching all actors - Improved actor error display to show crash messages - Fixed deployment URL formatting by properly extracting the base URL - Added Posthog tracking for when users skip onboarding - Enhanced the namespace route to use the new actor count query for determining if actors exist - Improved error handling in the actor status label component to display more specific crash messages ### How to test? 1. Navigate to the getting started flow and verify the actor count is displayed correctly 2. Check that deployment URLs are properly formatted in the frontend setup 3. Trigger an actor crash and verify the error message is displayed correctly 4. Skip onboarding and confirm the Posthog event is captured ### Why make this change? The previous implementation was inefficient as it fetched all actors just to get a count. This change optimizes performance by providing a dedicated endpoint for actor counts. Additionally, the improved error handling and URL formatting enhance the user experience during onboarding by providing clearer feedback and properly formatted links.
PR Review: fix(dashboard): fetch properly actor countsSummaryThis PR optimizes the actor count fetching by introducing a dedicated query function and improves the onboarding UX with better error handling and URL formatting. The changes are solid overall, but there are several areas that need attention. 🔴 Critical Issues1. Performance Issue: Inefficient Actor Count ImplementationLocation: The new // TODO: fetch all actor names only to get the count is inefficient
const namesList = await client.actorsListNames({
namespace,
limit: 100, // ⚠️ Hardcoded limit
});
const names = Object.keys(namesList.names);
const data = await Promise.all(
names.map((name) =>
client.actorsList({
namespace,
name,
limit: 1,
}),
),
);Problems:
Recommendation: This needs a backend API endpoint that returns just the count (e.g.,
2. Type Safety Issue: Invalid Query OptionsLocation: const { data: actors } = useQuery({
...dataProvider.actorsCountQueryOptions(),
enabled: (builds?.length || 0) > 0,
maxPages: 1, // ⚠️ Invalid option for useQuery
refetchInterval: 2500,
});Problem: Fix: Remove the 🟡 Significant Issues3. Missing Error Handling for URL ParsingLocation: const deploymentUrl = useMemo(() => {
if (!config?.url) return null;
try {
const url = new URL(config.url);
url.pathname = "/";
return url.toString();
} catch {
return null; // ⚠️ Silent failure
}
}, [config?.url]);Issue: The URL parsing error is silently swallowed. If Recommendation: Log the error or surface it to the user: } catch (error) {
console.warn('Invalid deployment URL:', config.url, error);
return null;
}4. Potential Race Condition in useEffectLocation: useEffect(() => {
const success = async () => {
successfulBackendSetupEffect();
router.invalidate();
return navigate({
to: ".",
search: (s) => ({
...s,
onboardingSuccess: true,
}),
});
};
if (hasActors) {
success().catch((error) => {
console.error(error);
});
}
}, [hasActors, navigate, router]);Problem: Missing Fix: Either add it to dependencies or wrap it in a ref if it's meant to be stable. 5. Inconsistent Error Message FormattingLocation: .with(P.shape({ crashed: P.shape({ message: P.string }) }), (err) => (
<p>Actor crashed. {err.crashed.message} </p> // ⚠️ Extra space before closing tag
))Issues:
Recommendation: <p>Actor crashed: {err.crashed.message}</p>or <p>Actor crashed. {err.crashed.message}</p> {/* No extra space */}🟢 Minor Issues / Suggestions6. PostHog Event Naming ConventionLocation: posthog.capture("onboarding_skipped");Suggestion: Verify this follows your analytics naming conventions. Consider adding context: posthog.capture("onboarding_skipped", {
project: params.project,
namespace: params.namespace,
});7. Missing Import CleanupLocation: The PR removed some imports but there might be unused imports remaining. Consider running a linter to clean up. 8. Query Key ConsistencyLocation: queryKey: [{ namespace }, "actors", "count"] as QueryKey,Verify this query key structure is consistent with your other query keys in the codebase. Some use arrays of objects, some use flat arrays. ✅ Good Practices
🧪 Testing Recommendations
📋 Action ItemsMust Fix Before Merge:
Should Fix:
Consider:
Overall AssessmentThe PR makes meaningful improvements to the onboarding flow and introduces a dedicated actor count query. However, the implementation still has the performance issue it aims to solve (hardcoded limit + N+1 queries). The changes are functional but need refinement before merge. Recommendation: Request changes to fix the critical |

TL;DR
Added actor count query functionality and improved the onboarding experience with better error handling and deployment URL formatting.
What changed?
actorsCountQueryOptionsfunction to efficiently query the count of actorsHow to test?
Why make this change?
The previous implementation was inefficient as it fetched all actors just to get a count. This change optimizes performance by providing a dedicated endpoint for actor counts. Additionally, the improved error handling and URL formatting enhance the user experience during onboarding by providing clearer feedback and properly formatted links.