Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 2, 2026

Closes FRONT-906

@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
rivetkit-serverless Error Error Jan 3, 2026 0:08am
2 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

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.

@linear
Copy link

linear bot commented Jan 2, 2026

@claude
Copy link

claude bot commented Jan 2, 2026

Pull Request Review: Show Runner Pool Errors

Summary

This PR adds comprehensive error display functionality for runner pools in the dashboard. The implementation groups configurations by provider and endpoint, displays error states with visual indicators, and provides detailed error information through tooltips.

Code Quality & Best Practices ✅

Strengths

  • Good use of TypeScript patterns: The ts-pattern library is used effectively for discriminated union handling in RunnerPoolError
  • Proper memoization: The groupedConfigs computation is correctly memoized with appropriate dependencies
  • Clean component structure: Good separation of concerns with dedicated components (StatusCell, RunnerPoolError, ProviderRow)
  • Accessibility: Visual indicators (ping, warning icon) combined with tooltips provide good UX

Issues & Recommendations

1. Typo in Error Message (Line 207) 🐛

errors={hasAtLeastOneError ? "One or more providers	have errors" : ""}

There's a tab character between "providers" and "have". Should be a space:

errors={hasAtLeastOneError ? "One or more providers have errors" : ""}

2. Logic Error in hasAtLeastOneError (Lines 196-198) 🐛

const hasAtLeastOneError = groupedConfigs.some(
    (g) => Object.keys(g.runnerPoolErrors).length > 0,
);

This checks if the object has keys, not if any errors actually exist. An entry with value undefined would still count as an error. Should be:

const hasAtLeastOneError = groupedConfigs.some(
    (g) => Object.values(g.runnerPoolErrors).some(err => err !== undefined),
);

3. Inconsistent Key Usage (Line 253) ⚠️

{isExpanded && groupedConfigs.map((groupedConfig, idx) => (
    <ProviderRow key={idx} name={name} {...groupedConfig} />
))}

Using array index as a key is an anti-pattern in React. Consider using a stable identifier:

{isExpanded && groupedConfigs.map((groupedConfig) => (
    <ProviderRow
        key={`${groupedConfig.provider}-${groupedConfig.endpoint}`}
        name={name}
        {...groupedConfig}
    />
))}

4. Type Safety Improvement (Lines 427-435)

The getProviderName function could benefit from a type guard:

interface ProviderMetadata {
    provider: string;
}

function isProviderMetadata(metadata: unknown): metadata is ProviderMetadata {
    return metadata !== null &&
           typeof metadata === 'object' &&
           'provider' in metadata &&
           typeof metadata.provider === 'string';
}

function getProviderName(metadata: unknown): string {
    if (!isProviderMetadata(metadata)) {
        return "unknown";
    }
    return metadata.provider;
}

5. Dead Code (Line 423)

function getModal(metadata: unknown) {
    return "edit-provider-config";
}

This function always returns the same value and ignores the parameter. Either:

  • Remove the parameter if it's not needed
  • Implement proper modal selection based on metadata (like the old code did)

6. JSX Keys Are Correct (Lines 322-333) ✅

When mapping over errors in the tooltip, fragments have proper keys - good job!

Performance Considerations ⚡

Positive

  • Proper memoization: The expensive grouping operation is memoized
  • Conditional rendering: Expanded state prevents unnecessary rendering of child rows

Potential Improvements

  1. Default expanded state (Line 156): All groups start expanded (useState(true)). For lists with many items, consider defaulting to false
  2. Consider virtualization: If the list can grow very large, consider using a virtualization library like react-window

Security Concerns 🔒

No Critical Issues Found ✅

  • Proper escaping in tooltips (React handles this automatically)
  • No dangerous HTML insertion
  • No XSS vulnerabilities detected

Minor Consideration

  • Error messages from the backend are displayed directly to users. Ensure backend doesn't leak sensitive information in error messages (not a frontend issue, but worth noting)

Test Coverage 🧪

Missing Tests

This PR doesn't include any test files. Consider adding:

  1. Unit tests for error grouping logic:

    • Test that configs are grouped correctly by provider + endpoint
    • Test error detection logic
    • Test edge cases (empty configs, all errors, no errors)
  2. Component tests:

    • Test RunnerPoolError with different error types
    • Test StatusCell rendering logic
    • Test expand/collapse behavior

Additional Observations

Good Practices ✅

  1. Removed console.log (edit-runner-config.tsx:57): Good cleanup
  2. Simplified namespace lookup (connect-vercel-frame.tsx): Removed unnecessary abstraction
  3. Consistent formatting: Code follows project conventions

Minor Style Issues

  1. connect.tsx formatting changes: The changes from multi-line to single-line arrow functions are stylistic. Ensure this matches project preferences

Verdict

Overall: Good implementation with minor issues to address

Must Fix Before Merge 🔴

  1. Fix typo in error message (line 207)
  2. Fix hasAtLeastOneError logic (lines 196-198)

Should Fix 🟡

  1. Use stable keys instead of array index (line 253)
  2. Clarify/fix getModal function purpose (line 423)
  3. Consider adding tests

Nice to Have 🟢

  1. Improve type safety with type guards
  2. Consider default collapsed state for better performance
  3. Add proper TypeScript interfaces for metadata

Great work on adding comprehensive error display! The visual feedback with status indicators and detailed tooltips will significantly improve the user experience. Once the critical issues are addressed, this will be ready to merge.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 2, 2026

Graphite Automations

"Test" took an action on this PR • (01/02/26)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

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