Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 30, 2026

No description provided.

Copy link
Contributor Author

jog1t commented Jan 30, 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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 30, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@4081

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@4081

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@4081

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@4081

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@4081

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@4081

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@4081

@rivetkit/traces

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/traces@4081

@rivetkit/workflow-engine

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/workflow-engine@4081

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4081

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@4081

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@4081

commit: fdf54ec

@claude
Copy link

claude bot commented Jan 30, 2026

PR Review: Workflows UI

This PR adds a comprehensive workflow visualization UI to the dashboard. Overall, this is an impressive implementation with sophisticated SVG-based rendering and interactive features.

Strengths

  1. Excellent Visual Design - Clean UI with proper color coding, smooth zoom/pan controls, hover states, and a nice dot grid background
  2. Comprehensive Feature Set - Support for complex workflow structures (loops, joins, races, branches), detailed/compact views, delta time display
  3. Good Type Safety - Well-structured TypeScript types with proper discriminated unions
  4. Code Organization - Logical separation into three files with constants defined at the top

Issues and Concerns

1. Memory and Performance (High Priority)

Issue: Large sample data (350+ lines) is bundled directly in the component file (actor-workflow-tab.tsx:13-359).

Impact: Adds approximately 15KB to bundle unnecessarily. Will be dead code when real API is implemented.

Recommendation: Move to separate mock file, remove entirely, or use minimal sample (5-10 nodes).

2. Accessibility Issues (Medium Priority)

Issue: Interactive SVG elements lack proper accessibility (workflow-visualizer.tsx:918-926)

  • No keyboard navigation (cannot tab between nodes)
  • No ARIA labels or roles
  • Screen readers will not understand structure
  • No focus management

Recommendations:

  • Add tabIndex={0} and role=button to interactive elements
  • Implement keyboard handlers (Enter/Space, Arrow keys)
  • Add aria-label describing each node
  • Consider accessible alternative view

3. Hardcoded Magic Values

Values like 500 (deltaSpace threshold) and 24 (spacing) lack explanation. Extract to named constants with comments.

4. Potential Layout Bugs

Division by zero not handled in parseAndLayout (workflow-visualizer.tsx:1218). Add safety checks with Math.max.

5. Missing Error Boundaries

Complex calculations could fail. Wrap in error boundary to prevent breaking entire actor details page.

Suggestions

  1. Performance: Memoize Connection component to avoid re-rendering all connections on hover
  2. State Management: Use useReducer for related state to prevent race conditions
  3. Add skeleton loaders for real API data loading states
  4. Add runtime type guards instead of type casting

Potential Bugs

  1. Mouse Up Outside Canvas: isPanning stays true if mouse released outside
  2. Scroll Lock: e.preventDefault() on wheel might trap users
  3. Text Overflow: String slicing at 18 chars could fail with multi-byte characters

Test Coverage

Missing: No tests for this complex component. Consider adding unit tests for parseAndLayout, edge case tests, interaction tests, and accessibility tests.

Overall Assessment

High-quality implementation with excellent visual design. Main concerns: accessibility, sample data bundle size, edge case handling, and test coverage.

Recommendation: Approve with changes requested

Great work on the visualization system! Once accessibility issues are addressed and sample data removed, this will be an excellent addition to the dashboard.

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