Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Sep 16, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-express Building Building Preview Comment Sep 16, 2025 9:46am

@graphite-app
Copy link

graphite-app bot commented Sep 16, 2025

Merge activity

  • Sep 16, 9:46 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Sep 16, 9:47 AM UTC: CI is running for this pull request on a draft pull request (#1248) due to your merge queue CI optimization settings.
  • Sep 16, 9:48 AM UTC: Merged by the Graphite merge queue via draft PR: #1248.

Copy link
Member Author

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 either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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 Sep 16, 2025

Claude finished @NathanFlurry's task —— View job


Code Review for Inspector URL Update

I've analyzed the changes in this PR that update the inspector URL from studio.rivet.dev to inspect.rivet.dev. Here's my comprehensive review:

Todo List

  • Read and analyze changed files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide comprehensive feedback

Summary of Changes

This PR updates the inspector URL across the codebase:

  • Changes default inspector URL from https://studio.rivet.dev to https://inspect.rivet.dev
  • Adds both old and new URLs to CORS origins for backward compatibility
  • Updates documentation references from "Rivet Studio" to "Rivet Inspector"

Code Quality & Best Practices ✅

Excellent: The changes follow good practices:

  • Maintains backward compatibility by keeping both URLs in CORS origins
  • Consistent naming throughout (studio → inspector)
  • Clear separation of concerns between config and utils
  • Good use of TypeScript typing with zod validation

Potential Issues ⚠️

Minor: Consider cleanup timeline

  • The old https://studio.rivet.gg URL is still in CORS origins (packages/rivetkit/src/inspector/config.ts:28)
  • While this maintains backward compatibility, consider documenting when this legacy URL can be removed

Documentation:

  • The change from "Like Postman" to "Like Chrome DevTools" in the README is more accurate for an inspector tool
  • Consider updating any other documentation that might reference the old studio URL

Performance Considerations ✅

No impact:

  • URL string changes have negligible performance impact
  • CORS origin array lookup remains O(n) but with minimal additional overhead
  • URL construction in getInspectorUrl() is still efficient

Security Assessment ✅

Strong security practices maintained:

  • CORS configuration properly restricts origins to known inspector domains
  • Timing-safe secret comparison using crypto.timingSafeEqual() in compareSecrets()
  • Proper authorization token validation with Bearer scheme
  • Both URLs use HTTPS, maintaining secure communication
  • Vercel preview domain pattern matching is appropriately restrictive (rivet-dev.vercel.app)

Good: The addition of the new URL to CORS origins maintains the allowlist approach rather than wildcard permissions.

Test Coverage Implications ⚠️

Consideration needed:

  • Tests should verify both URLs work in CORS origins during transition period
  • Integration tests should validate inspector URL generation with new domain
  • Consider adding tests that verify the inspector URL format and token parameters

Recommendation: Add test cases covering:

// Example test scenarios needed
expect(getInspectorUrl(config)).toMatch(/^https:\/\/inspect\.rivet\.dev/);
expect(corsOrigins).toContain("https://inspect.rivet.dev");
expect(corsOrigins).toContain("https://studio.rivet.gg"); // during transition

Additional Recommendations

  1. Migration strategy: Document the timeline for removing the old studio URL from CORS origins
  2. Monitoring: Consider logging or metrics to track usage of old vs new URLs during transition
  3. Version compatibility: Ensure this change is compatible with existing client applications

Overall Assessment ✅

This is a well-executed URL migration that:

  • Maintains backward compatibility during transition
  • Updates branding consistently across the codebase
  • Preserves all security measures
  • Has minimal risk of breaking existing functionality

The changes are safe to merge and demonstrate good engineering practices for URL migrations.


graphite-app bot pushed a commit that referenced this pull request Sep 16, 2025
@graphite-app graphite-app bot closed this Sep 16, 2025
@graphite-app graphite-app bot deleted the 09-16-chore_core_update_inspector_url branch September 16, 2025 09:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants