Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 2, 2026

TL;DR

Added error handling and display for Actors in the frontend UI.

What changed?

  • Added error field to Actor data structure and query options
  • Created new actorErrorQueryOptions for retrieving Actor errors
  • Moved RunnerPoolError component from runner-config-table to actor-status-label for reuse
  • Added ActorError component to display different types of Actor errors
  • Updated GuardConnectableInspector to show appropriate error messages based on Actor status
  • Changed "Stopped" status label to "Destroyed" for better clarity
  • Enhanced Actor status determination to consider error state as "crashed"

How to test?

  1. Create an Actor that encounters an error (e.g., by configuring an invalid serverless endpoint)
  2. Observe the error message displayed in the Actor UI
  3. Verify different error types are properly formatted and displayed
  4. Check that the Actor status correctly shows as "Crashed" when an error is present

Why make this change?

This change improves the developer experience by providing clearer feedback when Actors fail to start or encounter errors. Previously, error information was limited, making it difficult to diagnose issues. Now, specific error messages are displayed based on the error type, helping users understand and resolve problems more efficiently.

@vercel
Copy link

vercel bot commented Jan 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
rivet-inspector Ready Ready Preview, Comment Jan 3, 2026 0:08am
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 3, 2026 0:08am
rivet-site Ignored Ignored Preview Jan 3, 2026 0:08am
rivetkit-serverless Skipped Skipped Jan 3, 2026 0:08am

Copy link
Contributor Author

jog1t commented Jan 2, 2026

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.

@claude
Copy link

claude bot commented Jan 3, 2026

PR Review: feat(dash): surface actor errors in ui

Summary

This PR enhances the frontend UI to display Actor errors, improving the developer experience when debugging Actor failures. The implementation adds error handling and display logic across several components.

Code Quality & Best Practices

✅ Strengths

  • Good code reuse: The RunnerPoolError component was successfully extracted from runner-config-table.tsx and moved to actor-status-label.tsx for reuse
  • Type safety: Proper use of TypeScript types with Rivet.ActorError and Rivet.RunnerPoolError
  • Pattern matching: Excellent use of ts-pattern for handling different error types with exhaustive matching
  • Consistent status logic: Error state is now properly integrated into the getActorStatus function
  • Better UX: Changing "Stopped" to "Destroyed" improves clarity (line 13 in actor-status-label.tsx)

🔍 Observations

1. Status Determination Priority (queries/index.ts:128-130)
The error check happens first in getActorStatus, meaning an Actor with an error will always show as "crashed" even if it has other timestamps set. This seems correct, but worth noting:

if (error) {
    return "crashed";
}

Consider: Should an Actor with both error and destroyedAt show as "crashed" or "stopped"? The current logic prioritizes "crashed", which seems reasonable for debugging.

2. Query Structure (default-data-provider.tsx:199-204)
The new actorErrorQueryOptions is well-structured:

actorErrorQueryOptions(actorId: ActorId) {
    return queryOptions({
        ...this.actorQueryOptions(actorId),
        select: (data) => data.error,
    });
}

However, this creates a separate query subscription. If the parent actorQueryOptions is already being used elsewhere, consider whether the data is already cached.

3. Exhaustive Pattern Matching (actor-status-label.tsx:76-97)
The ActorError component uses .exhaustive(), which is excellent for type safety:

return match(error)
    .with(P.string, (errMsg) =>
        match(errMsg)
            .with("no_capacity", () => (...))
            .exhaustive(),
    )
    .with(P.shape({ runnerPoolError: P.any }), ...)
    .with(P.shape({ runnerNoResponse: P.any }), ...)
    .exhaustive();

Potential Issue: The inner match on P.string only handles "no_capacity". If other string error types exist in the backend, this will throw a runtime error. Consider adding .otherwise() instead of .exhaustive() for the inner match:

.with(P.string, (errMsg) =>
    match(errMsg)
        .with("no_capacity", () => (...))
        .otherwise(() => <span>Unknown error: {errMsg}</span>),
)

Potential Bugs & Issues

⚠️ Medium Priority

1. Error Display in UnavailableInfo (guard-connectable-inspector.tsx:68-74)
The error display wraps QueriedActorError in a <p> tag:

<Info>
    <p>Actor is unavailable.</p>
    <p>
        <QueriedActorError actorId={actorId} />
    </p>
</Info>

If QueriedActorError returns null (when isError || !error), this creates an empty paragraph. Consider:

.with("crashed", () => {
    const errorContent = <QueriedActorError actorId={actorId} />;
    return (
        <Info>
            <p>Actor is unavailable.</p>
            {errorContent && <p>{errorContent}</p>}
        </Info>
    );
})

Or rely on CSS to handle empty paragraphs if that's acceptable.

2. RunnerPoolError Return Type (actor-status-label.tsx:112-153)
The RunnerPoolError component returns different types:

  • null for nullish errors
  • String messages for all other cases
  • Uses .exhaustive() which is good

The function signature should be explicit about the return type:

export function RunnerPoolError({
    error,
}: {
    error: Rivet.RunnerPoolError | undefined;
}): string | null {
    // ...
}

However, looking at the usage context (line 88), it's used inside JSX <span>, so it's being rendered. The current implementation is fine, but the return type inconsistency (React component returning strings/null) is unusual.

Performance Considerations

⚡ Minor Concerns

1. Polling Interval (guard-connectable-inspector.tsx:42)

const { data: status } = useQuery({
    ...useDataProvider().actorStatusQueryOptions(actorId),
    refetchInterval: 1000,
});

1-second polling is reasonable for status updates, but ensure this doesn't cause performance issues with many Actors on screen. Consider:

  • Using a longer interval (2-3 seconds) for non-critical status checks
  • Implementing WebSocket updates for real-time status changes (if not already in place)

2. Multiple Query Subscriptions
The actorErrorQueryOptions, actorStatusAdditionalInfoQueryOptions, and actorQueryOptions all query the same underlying data. React Query's cache will deduplicate these, but be aware that:

  • Multiple components subscribing to different slices of the same data is efficient with React Query
  • The select function creates derived state, which is re-computed on each update

Security Concerns

✅ No Critical Issues Found

1. Error Message Display
Error messages from the backend are displayed directly to users:

  • serverlessHttpError.body (line 138)
  • serverlessConnectionError.message (line 141)
  • serverlessInvalidPayload.message (line 147)

Question: Are these messages sanitized on the backend? If these come from user-controlled serverless endpoints, there's a potential for XSS if the messages aren't sanitized. React's default escaping should handle this, but verify that:

  1. Error messages from external sources are sanitized
  2. No dangerouslySetInnerHTML is used elsewhere with this data

Test Coverage

📝 Suggestions

The PR doesn't include tests. Consider adding:

1. Unit tests for getActorStatus

describe('getActorStatus', () => {
    it('should return crashed when error is present', () => {
        const actor = {
            createdAt: '2024-01-01',
            startedAt: '2024-01-01',
            destroyedAt: null,
            sleepingAt: null,
            pendingAllocationAt: null,
            rescheduleAt: null,
            error: 'no_capacity',
        };
        expect(getActorStatus(actor)).toBe('crashed');
    });
    
    // Test that error takes precedence over other states
    it('should return crashed even when started', () => {
        const actor = {
            createdAt: '2024-01-01',
            startedAt: '2024-01-01',
            destroyedAt: null,
            sleepingAt: null,
            pendingAllocationAt: null,
            rescheduleAt: null,
            error: { runnerPoolError: 'internal_error' },
        };
        expect(getActorStatus(actor)).toBe('crashed');
    });
});

2. Component tests for error display

  • Test that ActorError renders correctly for each error type
  • Test that RunnerPoolError handles all error variants
  • Test that UnavailableInfo displays appropriate messages for each status

Additional Recommendations

1. Error Logging
Consider adding client-side error logging when an Actor error is encountered:

export function ActorError({ error }: { error: Rivet.ActorError }) {
    // Log error for debugging/monitoring
    console.error('[Actor Error]', error);
    
    return match(error)
        // ... existing code
}

2. Documentation
Add JSDoc comments to the new components:

/**
 * Displays Actor error messages based on error type.
 * Handles no_capacity, runnerPoolError, and runnerNoResponse errors.
 */
export function ActorError({ error }: { error: Rivet.ActorError }) {
    // ...
}

3. Accessibility
Error messages should be announced to screen readers. Consider wrapping errors in an ARIA live region:

<div role="alert" aria-live="polite">
    <ActorError error={error} />
</div>

Conclusion

Overall, this is a solid PR that improves the developer experience significantly. The code quality is good with appropriate use of TypeScript and pattern matching. The main concerns are:

  1. Action Required: Fix the inner .exhaustive() in ActorError to handle unknown string error types
  2. Consider: Add unit tests for the new status logic
  3. Consider: Verify backend sanitization of error messages displayed to users
  4. Nice to have: Add accessibility improvements for error announcements

Great work on improving error visibility in the UI! 🎉

@jog1t jog1t force-pushed the 01-03-feat_dash_surface_actor_errors_in_ui branch from eaf51c1 to d9ea998 Compare January 3, 2026 00:05
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless January 3, 2026 00:05 Inactive
@claude
Copy link

claude bot commented Jan 3, 2026

PR Review: feat(dash): surface actor errors in ui

Thank you for this improvement! This PR adds valuable visibility into actor failures.

Priority Issues

  1. Critical - Status Determination Logic (queries/index.ts:128-130)

    • Error check happens before reschedule check
    • If actor has both error and rescheduleAt, shows crashed instead of crash-loop
    • Recommendation: Consider showing crash-loop when rescheduleAt is set
  2. Important - Missing Error Handling (actor-status-label.tsx:76-98)

    • ActorError uses .exhaustive() but only handles no_capacity string error
    • Will throw at runtime if other string error types exist
    • Recommendation: Add .otherwise() fallback for unknown string errors
  3. Important - Security (actor-status-label.tsx:138)

    • HTTP error body displayed directly to users
    • Could leak internal errors, stack traces, or system info
    • Recommendation: Sanitize or limit error body length

Other Findings

Performance (guard-connectable-inspector.tsx:47)

  • 1-second polling could be aggressive with many actors
  • Consider 2-3 seconds or exponential backoff

Test Coverage

  • Missing tests for error rendering, status determination, message formatting

Strengths

  • Good code reusability moving RunnerPoolError to shared location
  • Comprehensive ts-pattern exhaustive matching
  • Improved UX surfacing errors directly
  • Better terminology: Stopped to Destroyed

Overall solid improvement! Main concerns are status determination edge cases and potential information disclosure.

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