Skip to content

feat(node-sdk): surface app data change callbacks and the update guard - #4017

Merged
tylerhawkes merged 1 commit into
tyler/appdata-cb-node-bindingsfrom
tyler/appdata-cb-node-sdk
Aug 20, 2026
Merged

feat(node-sdk): surface app data change callbacks and the update guard#4017
tylerhawkes merged 1 commit into
tyler/appdata-cb-node-bindingsfrom
tyler/appdata-cb-node-sdk

Conversation

@tylerhawkes

@tylerhawkes tylerhawkes commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Stack

Merge bottom-up — each PR is based on its parent below.
Android is the priority path: #4019#4011#4012#4016.

Already merged: #4018 (proto regen). Related follow-up, independent of this stack: #4020.


Surfaces the unstable app-data change callback on the node SDK.

Public surface

const client = await Client.create(signer, {
  unstableChangeCallbacks: {
    appData: async (change) => { /* merge and republish */ },
  },
});

The registry is an optional object rather than a bare callback, so handlers for the other mutable fields land as further optional properties.

The binding-level UnstableChangeCallbacks is only constructed when a handler is actually supplied — an empty registry would make the core snapshot app_data on every processed message for no benefit.

AppDataChange is re-exported from the SDK root so callers can type their handler.

Not included

browser-sdk. It runs the client in a Web Worker and passes options across a structured clone, which cannot carry a function; wiring it needs a worker→main reverse-RPC that does not exist yet, plus a decision on whether awaiting the main thread from inside the worker's processing loop is an acceptable stall risk. See #4014.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AdY7WKkNbJmdzpmUWvW1my

Note

Surface app data change callbacks and compare-and-swap update guard in the Node SDK

  • Adds UnstableChangeCallbacks to the Node bindings, letting JS callers register an async appData change callback at client creation time via OtherOptions.unstableChangeCallbacks.appData.
  • Introduces AppDataChange (carrying group_id, old_value, new_value) as a JS-visible type exported from the SDK.
  • Bridges JS callbacks to the core client via a ThreadsafeFunction; callback failures are logged and do not abort message processing.
  • Extends Group.updateAppData and UpdateAppDataOptions to accept an optional expectedValue/expectedAppData for compare-and-swap semantics on opaque app data.
📊 Macroscope summarized db23929. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

@tylerhawkes
tylerhawkes requested a review from a team as a code owner August 19, 2026 16:40
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Claude finished @tylerhawkes's task —— View job


PR Review - App Data Change Callbacks

Progress

  • Read changed files
  • Analyze implementation
  • Provide review feedback

Review Date: August 19, 2026, 8:48 PM UTC

This PR cleanly surfaces the app data change callback mechanism from the Node bindings to the SDK layer. The implementation is well-structured and follows established patterns. I have a few observations:

✅ Strengths

  1. Good API design - Using an object for unstableChangeCallbacks rather than a bare callback makes the API extensible for future mutable fields (name, description, etc.)

  2. Performance conscious - The conditional construction of UnstableChangeCallbacks only when a handler is actually provided (sdks/js/node-sdk/src/utils/createClient.ts:95-97) avoids unnecessary overhead

  3. Clear documentation - The JSDoc on updateAppData and unstableChangeCallbacks clearly explains the compare-and-swap semantics and callback behavior

  4. Error handling - The bindings layer properly catches and logs callback failures without derailing message processing (bindings/node/src/client/change_callbacks.rs:91-98)

🔍 Observations

  1. Unresolved review comment - The macroscopeapp review identified an issue with undefined handling for expectedAppData (sdks/js/node-sdk/src/Group.ts:104). When a caller expects appData to be unset and passes undefined, it's indistinguishable from "no guard" (last-writer-wins), which could silently overwrite a concurrent value. Consider accepting null as a distinct sentinel for "expect unset".

  2. Test coverage - Only basic updateAppData usage is tested (sdks/js/node-sdk/test/Group.test.ts:300). Consider adding tests for:

    • The expectedAppData compare-and-swap parameter (both success and conflict cases)
    • The unstableChangeCallbacks.appData callback being invoked
    • Callback error handling
  3. Type export location - AppDataChange is exported from the SDK root (sdks/js/node-sdk/src/index.ts:21), which is good for discoverability


macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Aug 19, 2026
@macroscopeapp

macroscopeapp Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR introduces new user-facing behavior: a callback mechanism for app data changes and a compare-and-swap parameter for updateAppData. While well-scoped and additive, new feature capabilities warrant human review even when the author owns the affected files.

No code changes detected at db23929. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from a87b535 to a188e1c Compare August 19, 2026 16:48
@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from a188e1c to 15dc738 Compare August 19, 2026 17:20
@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from 15dc738 to 5ba3c44 Compare August 19, 2026 17:51
@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from 5ba3c44 to 1e34c2a Compare August 19, 2026 18:26
@macroscopeapp
macroscopeapp Bot dismissed their stale review August 19, 2026 18:27

Dismissing prior approval to re-evaluate 1e34c2a

@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from 1e34c2a to 36f53bc Compare August 19, 2026 18:51
@tylerhawkes
tylerhawkes changed the base branch from tyler/appdata-cb-android to tyler/appdata-cb-node-bindings August 19, 2026 18:51
@tylerhawkes tylerhawkes changed the title feat(node-sdk): surface app data change callbacks in client options feat(node-sdk): surface app data change callbacks and the update guard Aug 19, 2026
@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from 36f53bc to 9e03a38 Compare August 19, 2026 19:16
@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from 9e03a38 to a1e75f4 Compare August 19, 2026 20:03
*/
async updateAppData(appData: string) {
return this.#conversation.updateAppData({ value: appData });
async updateAppData(appData: string, expectedAppData?: string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 High src/Group.ts:104

updateAppData cannot detect a conflict when the caller expects appData to be unset: passing undefined omits expectedValue, so a concurrent value can be silently overwritten. Accept a distinct unset sentinel such as null and forward it to the binding as expectedValue.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @sdks/js/node-sdk/src/Group.ts around line 104:

`updateAppData` cannot detect a conflict when the caller expects `appData` to be unset: passing `undefined` omits `expectedValue`, so a concurrent value can be silently overwritten. Accept a distinct unset sentinel such as `null` and forward it to the binding as `expectedValue`.

@tylerhawkes
tylerhawkes force-pushed the tyler/appdata-cb-node-sdk branch from a1e75f4 to db23929 Compare August 19, 2026 20:47
@tylerhawkes
tylerhawkes merged commit 39ffd5d into main Aug 20, 2026
69 of 79 checks passed
@tylerhawkes
tylerhawkes deleted the tyler/appdata-cb-node-sdk branch August 20, 2026 14:53
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