Skip to content

fix(rivetkit): expose client actor metadata#5016

Draft
NathanFlurry wants to merge 1 commit into
counter-latency/unbounded-dispatch-channelsfrom
counter-latency/client-protocol-polish
Draft

fix(rivetkit): expose client actor metadata#5016
NathanFlurry wants to merge 1 commit into
counter-latency/unbounded-dispatch-channelsfrom
counter-latency/client-protocol-polish

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Code Review

Overview

This PR exposes the actor metadata (specifically actorId) on the client-side ActorConnRaw so callers can read it after connecting. It also:

  • Adds a CLAUDE.md note about client/server protocol compatibility direction.
  • Adds the actor field to both the inline and named Error schemas in asyncapi.json.
  • Pre-populates #actorId at construction time for getForId connections.

Issues

Test doesn't cover the new code path (medium)

The new constructor code:

if ("getForId" in actorResolutionState) {
    this.#actorId = actorResolutionState.getForId.actorId;
}

only runs for getForId queries. But the test uses getOrCreate:

const handle = client.counter.getOrCreate(["test-conn-actor-id"]);
const connection = handle.connect();
await connection.ready;
expect(connection.actorId).toBe(await handle.resolve()); // passes via Init message, not the new path

The test validates the existing Init-message path, not the new constructor pre-population. The primary new behavior — that actorId is available before connection.ready for getForId handles — goes untested. A test like this would cover it:

const actorId = await client.counter.getOrCreate(["test-conn-actor-id"]).resolve();
const handle = client.counter.getForId(actorId);
const connection = handle.connect();
// actorId is already known before the WebSocket handshake
expect(connection.actorId).toBe(actorId);
await connection.ready;
expect(connection.actorId).toBe(actorId);
await connection.dispose();

JSDoc is stale (low)

The actorId getter is still marked @internal with the comment "for testing purposes", but this PR's intent is to expose it as a real client feature. If it is now public API, update the JSDoc (remove @internal, update the description); if it remains internal, the PR description should clarify why a dedicated test is validating it.

/**
 * Get the actor ID (for testing purposes).
 * @internal
 */
get actorId(): string | undefined {

asyncapi.json — generation type inconsistency (nit)

Every other int64 field in the file is typed as { "type": "integer", "format": "int64" }. The new generation field uses anyOf: [{ type: "number" }, { type: "integer", format: "int64" }]. type: "number" is a superset of integers in JSON Schema, so the first branch is redundant for strict senders. If this is intentional for backward-compat with older protocol versions that sent a plain float, a comment would help; otherwise align with the established int64 pattern.

asyncapi.json — duplicated actor schema definition (nit)

The actor object is defined identically inline in the message body and as a named component in the schemas section. Consider making the inline definition $ref the named schema to keep them in sync automatically, which is the standard AsyncAPI pattern.


What looks good

  • The constructor pre-population is correct for getForId connections. Since the actor ID is statically known at construction time, it's safe to set it early, and the existing #clearResolvedActorIdentity + Init-message flow handles reconnects correctly.
  • The CLAUDE.md note is concise and follows the one-line bullet convention.
  • The new required / additionalProperties: false on the actor schema is consistent with the rest of the file.

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.

1 participant