Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

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 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.

@NathanFlurry NathanFlurry mentioned this pull request Jan 19, 2026
@claude
Copy link

claude bot commented Jan 19, 2026

Code Review - PR #3977: Make actions optional in RivetKit

Summary

This PR makes the actions field optional in actor configurations and updates related code to handle the absence of actions gracefully. This is a quality-of-life improvement that removes boilerplate for actors that do not use actions.


✅ Positive Changes

  1. Removes unnecessary boilerplate: Several actors in the codebase currently use actions: {}, which is redundant. Making it optional improves developer experience.

  2. Consistent null-coalescing: The implementation properly uses ?? {} pattern in both places where actions are accessed (mod.ts:206 and mod.ts:519).

  3. Type safety maintained: The optional type actions?: TActions correctly maintains type safety while allowing omission.

  4. Documentation updated: The schema documentation correctly describes the new behavior.


🔍 Issues & Concerns

1. Inconsistent formatting changes (Minor - Cosmetic)

The PR includes significant formatting changes to DocActorOptionsSchema and DocActorConfigSchema that appear to be auto-formatter output. While these improve readability, they make the diff harder to review.

Location: src/actor/config.ts:630-810

Recommendation: Per CLAUDE.md, formatters should not be run manually. These changes seem intentional for consistency, but they obscure the actual functional changes. Consider keeping formatting changes in a separate commit/PR if possible, or accept this as a one-time improvement.


2. Missing buildGatewayUrl implementation explanation (Minor - Unclear)

The PR adds a buildGatewayUrl implementation to the test driver without explanation.

Location: src/driver-test-suite/test-inline-client-driver.ts:240-242

Issue: This change appears unrelated to making actions optional. It is not clear from the PR title or description why this was added.

Recommendation: Either explain this change in the PR description or split it into a separate PR if unrelated.


3. Edge case: Action execution with undefined actions (Low Risk)

The code properly handles undefined actions. If this.#config.actions is explicitly set to null, it would convert to an empty object, but TypeScript prevents this scenario.

Verdict: This is fine - just defensive programming.


🧪 Test Coverage

Concern: The PR does not add explicit tests for actors without actions.

Existing Evidence: Multiple fixture actors already use actions: {}:

  • rawHttpNoHandlerActor (raw-http.ts:61-63)
  • rawHttpVoidReturnActor (raw-http.ts:65-71)
  • rawHttpHonoActor (raw-http.ts:73-124)
  • rawWebSocketBinaryActor (raw-websocket.ts:97-114)

Recommendation: Consider adding a test case that creates an actor with no actions field (not actions: {}), verifies that instance.actions returns an empty array, and verifies that calling a non-existent action throws ActionNotFound.


🎯 Recommendations

High Priority:

  1. Explain or separate the buildGatewayUrl change - This seems unrelated to the PR purpose

Medium Priority:
2. Add test coverage - Create a test for actors without the actions field

Low Priority:
3. Update existing fixtures - As a follow-up, consider removing actions: {} from existing fixture actors to demonstrate the new pattern


✨ Overall Assessment

Verdict: ✅ Approve with minor comments

This is a good quality-of-life improvement that:

  • Reduces boilerplate
  • Maintains backward compatibility
  • Is implemented correctly with proper null-coalescing

The main concerns are:

  1. The unexplained buildGatewayUrl addition (needs clarification)
  2. Missing explicit test coverage (nice-to-have)

The code changes are sound and follow TypeScript best practices.


📝 Code Quality Checklist

  • ✅ Type Safety: Properly typed with actions?: TActions
  • ✅ Null Safety: Correct use of ?? {} for null-coalescing
  • ✅ Backward Compatibility: Existing code with actions: {} continues to work
  • ✅ Documentation: Schema documentation updated
  • ⚠️ Test Coverage: Could benefit from explicit test case
  • ⚠️ PR Scope: Contains seemingly unrelated change (buildGatewayUrl)

) => void | Promise<void>;

actions: TActions;
actions?: TActions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line changes 'actions' from required to optional, which may cause issues if other parts of the codebase expect actions to always be defined. Ensure this change is intentional and that code using this property handles the undefined case properly. Additionally, the formatting changes throughout the file should be applied using Biome instead of manually to ensure consistency with project standards.

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@NathanFlurry NathanFlurry force-pushed the 01-19-chore_rivetkit_make_actions_optional branch from 0569449 to 5c681ce Compare January 19, 2026 19:36
@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 19, 2026

More templates

@rivetkit/virtual-websocket

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

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: d5019ff

@NathanFlurry NathanFlurry force-pushed the 01-19-chore_rivetkit_make_actions_optional branch from 5c681ce to f69dce5 Compare January 20, 2026 19:48
@NathanFlurry NathanFlurry force-pushed the 01-19-chore_rivetkit_make_actions_optional branch from f69dce5 to d5019ff Compare January 21, 2026 00:09
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 21, 2026

Merge activity

  • Jan 21, 12:11 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 21, 12:11 AM UTC: CI is running for this pull request on a draft pull request (#3988) due to your merge queue CI optimization settings.
  • Jan 21, 12:12 AM UTC: Merged by the Graphite merge queue via draft PR: #3988.

graphite-app bot pushed a commit that referenced this pull request Jan 21, 2026
@graphite-app graphite-app bot closed this Jan 21, 2026
@graphite-app graphite-app bot deleted the 01-19-chore_rivetkit_make_actions_optional branch January 21, 2026 00:12
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