You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reviewed the diff (excluding the auto-generated common/bare/generated/** protocol files, which are codegen output).
Summary
The core change, adding an id-based form to useActor alongside the existing key-based form, is well executed:
ActorOptions is correctly modeled as a discriminated union (ActorByKeyOptions | ActorByIdOptions) with ActorOptionsBase factored out for the shared name/params/enabled fields. The "id" in actor.opts narrowing in create() (rivetkit-typescript/packages/framework-base/src/mod.ts:470) is exhaustive and type-safe.
It builds entirely on the existing client.getForId() (already used elsewhere for engine-client/actor-conn resolution), so there's no new client-side plumbing or protocol surface. Low risk.
ActorByIdOptions correctly omits noCreate/createInRegion/createWithInput since id-based lookup always resolves an existing actor and never creates one, and the doc comments explain this well.
mod.typecheck.ts adds solid compile-time coverage, including a @ts-expect-error case asserting useActor({ name: "counter" }) (no key or id) fails to type-check.
Suggestions (non-blocking)
No runtime test for the new behavior. Coverage for the id-based path is type-check only (mod.typecheck.ts). There's no test exercising that create() actually calls client.getForId with the right args, or that switching an actor's opts between key-based and id-based (or vice versa) correctly changes the hash and triggers a reconnect via optsEqual. Worth adding if there's a harness for this package (I didn't find existing .test.ts files under react/ or framework-base/, so this may just follow existing convention for these packages).
Scope creep / unrelated changes bundled in. This diff also includes changes that look unrelated to "connect by id in useActor": the publish.yaml engine-binary version verification step, the tsup.config.ts shims flip (paired with tsup.browser.config.ts), resolveServerlessDrainGracePeriod in serverless/configure.ts, the dynamic-import cleanups in napi-runtime.ts/native.ts/wasm-runtime.ts, and the quickstart docs updates. These look like they came from the earlier "repair quickstart package startup" commit that's part of this branch. If that's intentional (stacked/rebased branch), fine, otherwise worth splitting so the PR title matches its contents.
Minor style nit:rivetkit-typescript/packages/framework-base/src/mod.ts:556 (defaultHashFunction's return statement) is a long single line. agent-format.mjs will likely reformat it but flagging in case it's not run before merge.
publish.yaml verify step: the new "Verify packaged engine binary versions" step (.github/workflows/publish.yaml:521) silently passes (failed stays 0) if none of the globbed binary paths exist, since unmatched globs just become literal strings that fail the [ -e ... ] check and continue. That means if packaging entirely failed to produce any binaries, this step wouldn't catch it. Might be worth asserting at least one binary was actually verified, though this may be acceptable if this step only runs after packaging is confirmed to have succeeded elsewhere.
Everything else
resolveServerlessDrainGracePeriod logic and its new unit tests in configure.test.ts check out against the boundary cases (exactly at the default threshold, very short lifespans, explicit override). The tsup.config.ts shims change is consistent with the new comment and tsup.browser.config.ts already existing with shims: false for the browser build.
No security concerns: getForId was already a public client capability; the hook just exposes it through React, and per the trust-boundary docs, authorization for actor ID access is expected to be enforced engine-side already (client to engine remains untrusted regardless of key vs. id resolution).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.