Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Dec 4, 2025

No description provided.

@vercel
Copy link

vercel bot commented Dec 4, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Dec 8, 2025 11:30pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 8, 2025 11:30pm
rivet-inspector Ignored Ignored Preview Dec 8, 2025 11:30pm
rivet-site Ignored Ignored Preview Dec 8, 2025 11:30pm

Copy link
Contributor Author

jog1t commented Dec 4, 2025


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 Dec 4, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: ce7695c

@claude
Copy link

claude bot commented Dec 4, 2025

Code Review - PR #3589: Add DevTools

This PR adds a new RivetKit DevTools package with a draggable floating button to access the Rivet Inspector. Overall, the implementation is well-structured with good separation of concerns. Here are my findings:


Strengths

  1. Good architecture: Clean separation between hooks (useDraggable, useCornerPosition), components, and main entry point
  2. Shadow DOM usage: Properly isolates the devtools UI from the host page styling
  3. Smart lazy loading: Devtools script is only loaded when enabled, reducing unnecessary overhead
  4. User experience: Draggable button with corner snapping provides a polished interaction
  5. Clean integration: Non-invasive integration with the main RivetKit client via optional config flag
  6. Build configuration: IIFE format is appropriate for browser injection

🔍 Issues & Concerns

Critical

  1. Security: Sensitive data exposure (rivetkit-typescript/packages/devtools/src/mod.tsx:68-76)

    • The devtools opens a URL with potentially sensitive data in query parameters (token, endpoint, namespace)
    • Tokens in URLs can leak via browser history, logs, and referrer headers
    • Recommendation: Use POST message API or store credentials in sessionStorage and retrieve them in the inspector page
  2. Type safety issue (rivetkit-typescript/packages/devtools/src/mod.tsx:57-58)

    • window.__rivetkit?.[0] assumes the first config is the correct one, but multiple clients could exist
    • No validation that the config has required fields before constructing URL
    • Recommendation:
      const config = window.__rivetkit?.find(c => c?.endpoint);
      if (!config?.endpoint) {
          console.error('RivetKit Devtools: No valid client config found');
          return;
      }
  3. Memory leak potential (rivetkit-typescript/packages/devtools/src/hooks/useDraggable.ts:143-168)

    • Animation promise chains without proper cleanup if component unmounts during animation
    • animationRef.current is set to null in cleanup, but ongoing promises may still execute
    • Recommendation: Track if component is mounted and guard state updates:
      const isMountedRef = useRef(true);
      useEffect(() => () => { isMountedRef.current = false; }, []);
      
      animation.finished.then(() => {
          if (!isMountedRef.current) return;
          // ... rest of code
      });

Medium Priority

  1. Race condition (rivetkit-typescript/packages/devtools/src/hooks/useDraggable.ts:158-164)

    • Using flushSync for state updates during animation is problematic
    • State updates inside animation callback can cause re-renders at unexpected times
    • Recommendation: Move state updates outside animation callback or use regular setState
  2. Accessibility issues (rivetkit-typescript/packages/devtools/src/components/DevButton.tsx)

    • Button lacks aria-label for screen readers
    • No keyboard navigation support for dragging (only pointer events)
    • Recommendation: Add aria-label="Open RivetKit Inspector" and consider keyboard drag support
  3. Error handling (rivetkit-typescript/packages/devtools/src/hooks/useDraggable.ts:165-168)

    • Empty catch block silently swallows all animation errors
    • Hard to debug if animation issues occur
    • Recommendation: At minimum log to console in development:
      .catch((err) => {
          animationRef.current = null;
          if (process.env.NODE_ENV === 'development') {
              console.warn('Animation cancelled:', err);
          }
      });
  4. localStorage usage without error handling (rivetkit-typescript/packages/devtools/src/hooks/useCornerPosition.tsx:15-18)

    • localStorage can throw in private browsing mode or when disabled
    • Recommendation: Wrap in try/catch
  5. Missing CSP consideration (rivetkit-typescript/packages/devtools/src/mod.tsx:24-26)

    • Using dangerouslySetInnerHTML for styles may violate strict CSP policies
    • Recommendation: Document CSP requirements or use alternative injection method

Low Priority

  1. TypeScript strict mode (rivetkit-typescript/packages/devtools/src/hooks/useCornerPosition.tsx:49)

    • Long line exceeds typical 100-character limit (minor style issue)
  2. Dependencies (rivetkit-typescript/packages/devtools/package.json:44)

    • motion library is listed as dependency but I don't see it imported in the reviewed files
    • Verify: Is this actually used? If not, remove it to reduce bundle size
  3. Build output (rivetkit-typescript/packages/devtools/tsup.config.ts:7-8)

    • Entry configuration syntax is unusual: {"src/mod.tsx": "mod.js"}
    • Standard would be: entry: ["src/mod.tsx"] with outDir controlling output
    • May cause confusion for maintainers

📊 Test Coverage

  • Missing: No unit tests for hooks or components
  • Recommendation: Add tests for:
    • useDraggable hook drag behavior and snapping logic
    • useCornerPosition hook corner calculation
    • DevButton component interactions

🎯 Performance Considerations

  1. Good: Lazy loading of devtools script only when enabled
  2. Good: Shadow DOM prevents style recalculation impact on host page
  3. Consider: The devtools button renders on every page load even when not used - could defer rendering until user hovers over trigger area

📝 Code Quality

  1. Consistent naming: Good use of descriptive function and variable names
  2. Type safety: Generally good TypeScript usage with proper types
  3. React best practices: Proper use of hooks, refs, and memoization
  4. Bundle size: IIFE bundle includes React/ReactDOM - ensure tree-shaking is working properly

🔒 Security Summary

  • High risk: Token exposure in URL query parameters
  • Medium risk: No validation on external script injection (though URL is hardcoded)
  • Low risk: Shadow DOM properly isolates content

✏️ Suggestions

  1. Consider adding a config option to disable devtools in production builds
  2. Add documentation for security implications of devtools in README
  3. Consider using a more secure communication method for credentials
  4. Add E2E tests for the full devtools integration flow

📋 Summary

The implementation is solid overall with good UX and clean architecture. The primary concern is security around credential handling in URLs. The animation logic could use some refinement to prevent edge case issues. With the security fix and minor improvements, this would be production-ready.

Recommendation: Address the critical security issue before merging, medium priority issues are nice-to-have improvements.

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 4, 2025

Graphite Automations

"Test" took an action on this PR • (12/04/25)

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