Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Dec 29, 2025

No description provided.

@vercel
Copy link

vercel bot commented Dec 29, 2025

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

4 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Dec 29, 2025 5:07pm
rivet-inspector Ignored Ignored Preview Dec 29, 2025 5:07pm
rivet-site Ignored Ignored Preview Dec 29, 2025 5:07pm
rivetkit-serverless Skipped Skipped Dec 29, 2025 5:07pm

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 29, 2025 17:06 Inactive
Copy link
Contributor Author

jog1t commented Dec 29, 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.

@jog1t jog1t force-pushed the 12-29-refactor_vite_hono_dev_server branch from 1341b79 to f5a26c8 Compare December 29, 2025 17:07
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 29, 2025 17:07 Inactive
@claude
Copy link

claude bot commented Dec 29, 2025

Pull Request Review

This PR refactors the actor-actions example to use a unified Vite + Hono dev server instead of the previous split setup with concurrently running frontend and backend servers.

🎯 Summary

Changes:

  • Consolidates dev server from dual frontend/backend setup to single Vite + Hono server
  • Moves frontend files from src/frontend/ to frontend/
  • Moves backend registry from src/backend/registry.ts to src/registry.ts
  • Adds new src/server.tsx with Hono server and JSX-based HTML template
  • Updates Vite config to support dual-mode builds (client + server)
  • Simplifies package.json scripts

❌ Critical Issues

1. Missing HTML file and broken dev server setup (src/server.tsx:14)

The server references /static/client.js in production mode, but the vite config outputs static/main.js:

{import.meta.env.PROD ? (
  <script type='module' src='/static/client.js'></script>  // ❌ Wrong filename

The vite.config.ts specifies:

entryFileNames: 'static/main.js',  // Should be main.js, not client.js

Fix: Change client.js to main.js in server.tsx:14

2. Inconsistent indentation (src/server.tsx:10)

Line 10 uses spaces instead of hard tabs, violating the project's rustfmt.toml style:

app.get("/", async (c) => {
	 return c.html(  // ❌ Space indentation here

Fix: Use hard tabs consistently throughout the file (see CLAUDE.md: "Hard tabs for Rust formatting")

3. Missing newline at EOF (frontend/index.css:124)

The CSS file is missing a trailing newline, which violates common coding standards.


⚠️ Code Quality Issues

4. Port mismatch in client (frontend/App.tsx:9)

The client still references port 6420:

const client = createClient<typeof registry>("http://localhost:6420");

But according to package.json, the frontend port is 5173. This will likely cause connection failures. The Hono server mounts the registry at /api/rivet, so the client should use relative URLs or connect to the correct Vite dev server port.

Recommendation: Use a relative path or verify the correct port configuration.

5. Removed concurrently dependency still in package.json (package.json:21)

The concurrently dependency is no longer needed since we're not running dual servers, but it's still listed in devDependencies.

Fix: Remove concurrently from devDependencies.

6. Dependency organization (package.json:13-28)

The devDependencies mixing violates conventional organization:

  • react, react-dom, hono, and @rivetkit/react should be in dependencies, not devDependencies
  • These are runtime dependencies needed for the application to function

Fix: Move runtime packages to dependencies.


🔍 Configuration Issues

7. Vite config formatting (vite.config.ts:21)

Inconsistent indentation - line 21 uses hard tabs while surrounding lines use spaces.

Fix: Use consistent spacing (spaces for TypeScript/JavaScript files, per common convention).

8. Build script may fail silently (package.json:8)

The build script chains two vite builds:

"build": "vite build --mode client && vite build"

With emptyOutDir: false in client mode, there's a risk of file conflicts or stale artifacts.

Recommendation: Consider adding a clean step or document this behavior.


✅ Positive Aspects

  • Good consolidation reducing complexity
  • Proper separation of client/server build modes
  • Maintains type safety with registry imports
  • Test file correctly updated to new import path

🧪 Testing Recommendations

  1. Verify dev server works: Run npm run dev and ensure frontend loads correctly
  2. Test production build: Run npm run build && npm run serve to verify the production build works
  3. Verify client connection: Ensure the RivetKit client can connect to the mounted registry at /api/rivet
  4. Run existing tests: Confirm npm test passes with updated imports

📋 Action Items

Must fix before merge:

  1. Fix script src from /static/client.js to /static/main.js (src/server.tsx:14)
  2. Fix indentation in src/server.tsx to use hard tabs
  3. Add missing newline at EOF in frontend/index.css

Should fix:
4. Verify and fix client port configuration (frontend/App.tsx:9)
5. Remove unused concurrently dependency
6. Reorganize dependencies vs devDependencies

Nice to have:
7. Fix vite.config.ts indentation consistency
8. Add build output cleanup or document the emptyOutDir: false behavior


Overall, this is a good refactoring that simplifies the development setup, but it needs the critical bugs fixed (especially the client.js/main.js mismatch) before it can work correctly.

@claude
Copy link

claude bot commented Dec 29, 2025

PR Review: Vite × Hono Dev Server Refactor

Summary

This PR refactors the actor-actions example to use a unified Vite + Hono development setup, eliminating the need for running separate backend and frontend processes. The changes consolidate the development experience into a single server.


Positive Changes

  1. Improved Developer Experience: Consolidating from two separate processes into a single vite command simplifies development workflow.

  2. Better Project Structure: Moving frontend files from src/frontend/ to frontend/ creates clearer separation between frontend and backend code.

  3. Modern Tooling: Using @hono/vite-dev-server and @hono/vite-build provides better integration between Hono and Vite.

  4. Simplified Scripts: The package.json scripts are cleaner


Issues & Concerns

1. Hardcoded API URL in Frontend (Critical)

File: frontend/App.tsx:9

The frontend is hardcoded to http://localhost:6420 but the new dev server runs on Vite's default port (likely 5173) and the API is mounted at /api/rivet.

Fix Required: Use relative URLs instead: /api/rivet

2. Incorrect Output Path in Build Config (Critical)

File: vite.config.ts:13 outputs static/main.js but src/server.tsx:14 references static/client.js.

This will cause a 404 in production builds.

Fix Required: Either change build output to client.js OR update server to reference main.js

3. Indentation Inconsistency (Minor)

File: src/server.tsx:10 - Line 10 uses space instead of tab, inconsistent with the project's hard tabs convention (CLAUDE.md).

4. Missing Newline at End of File (Minor)

File: frontend/index.css:124 - Missing newline at EOF

5. FIXME Comment (Low Priority)

File: src/server.tsx:26 - Consider resolving before merge or adding context about why registry.serve() cannot be used yet.


Recommendations Summary

Must Fix Before Merge:

  1. Fix client.js vs main.js mismatch (vite.config.ts:13 and server.tsx:14)
  2. Update hardcoded API URL in App.tsx:9 to /api/rivet
  3. Fix indentation in server.tsx:10 (use hard tabs)

Should Fix:
4. Add newline at end of index.css:124
5. Resolve or document FIXME at server.tsx:26


Overall Assessment

Solid refactor that improves DX, but 2-3 critical issues must be fixed before merge. The client.js/main.js mismatch will break production and the hardcoded API URL will prevent the app from working.

Recommendation: Request changes for critical issues, then approve once resolved.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 29, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: f5a26c8

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