Implement the data model for web renderers v0.9 by jacobsimionato · Pull Request #606 · google/A2UI

jacobsimionato · 2026-02-09T01:42:42Z

Refactor: Enhance DataModel robustness and type safety in web_core v0.9

This change refactors the DataModel in @a2ui/web_core/v0_9 to improve its robustness, type safety, and adherence to the A2UI protocol specification, particularly concerning data manipulation and path resolution.

Key changes include:

Improved DataModel.set path handling:
- Modified the set method to throw an error if an intermediate segment of a path points to a primitive value when attempting to set a nested property, preventing unexpected data corruption.
- Added a check to enforce numeric indices for array segments in paths, throwing an error for non-numeric access on arrays to maintain data model integrity.
- Updated documentation for the set method to explicitly describe the behavior when setting values to undefined for both objects (key removal) and arrays (index nullification).
Enhanced Type Safety:
- Updated the Subscription<T> interface to correctly type value and onChange callback parameters as T | undefined, reflecting potential undefined states in the data model and improving consumer type safety.
Test Coverage & Fixes:
- Added new test cases in data-model.test.ts to validate the error conditions for primitive path traversal and non-numeric array segment access.
- Resolved TypeScript compilation errors in web_core and lit by ensuring @types/node is correctly installed and by updating web_core/package.json to explicitly export v0_9 modules.
- Corrected implicit any type errors in test callbacks.
- Fixed a JSON syntax error in specification/v0_9/json/standard_catalog.json.
Documentation Updates:
- Updated specification/v0_9/docs/a2ui_protocol.md to reflect the refined data model behavior for undefined values and array index rules.

gemini-code-assist

Code Review

This pull request introduces a DataModel class for managing client-side state, complete with a comprehensive test suite. The implementation is robust, providing a reactive data store with path-based access and subscriptions. My review focuses on improving type safety, code clarity, and consistency. I've identified a significant type-safety issue in the Subscription interface that could lead to runtime errors, and I've provided a suggestion to fix it. I also found a minor redundancy in the notification logic and an inconsistency in handling undefined values that should be addressed or at least documented. Lastly, please note that the repository's contribution guidelines require the PR description to include the Pre-Review Checklist, which is currently missing.

renderers/web_core/src/v0_9/state/data-model.ts

jacobsimionato · 2026-02-09T02:26:42Z

I have addressed the feedback. Please take another look.

jacobsimionato · 2026-02-09T02:26:53Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces the v0.9 data model for web renderers, including the DataModel class implementation and a comprehensive test suite. The implementation is well-structured and the tests cover many cases.

I've found a critical issue in the set method's path traversal logic that could lead to incorrect behavior when a path contains a primitive value. I've provided a fix for this. I've also suggested an additional test case to cover this scenario and ensure the model behaves as expected.

The rest of the changes, including package updates and documentation, look good and are consistent with the new data model implementation.

renderers/web_core/src/v0_9/state/data-model.ts

gemini-code-assist · 2026-02-09T02:28:03Z

renderers/web_core/src/v0_9/state/data-model.test.ts

+    model.set('/foo', 'bar');
+    assert.strictEqual(count, 0);
+  });
+});


This is a great set of tests! To make it even more robust, I recommend adding a test case to verify the behavior of set when a path segment contains a primitive value. This will ensure the fix for the issue I mentioned in data-model.ts is covered and prevent regressions.

Here's a suggested test case:

Suggested change

});

it('overwrites primitive values in path when setting nested properties', () => {

model.set('/user/name', 'not an object');

assert.strictEqual(model.get('/user/name'), 'not an object');

// Now, try to set a nested property. This should overwrite the primitive.

model.set('/user/name/first', 'Alice');

assert.deepStrictEqual(model.get('/user/name'), { first: 'Alice' });

assert.strictEqual(model.get('/user/name/first'), 'Alice');

});

});

References

The style guide requires that code changes should have tests. While tests are present, this comment suggests adding a test for a critical edge case that was missed, improving test coverage and robustness. ^(link)

Please add a test case for this, but verify failure as explained above.

jacobsimionato · 2026-02-09T02:39:19Z

I have addressed the feedback. Please take another look.

jacobsimionato · 2026-02-09T02:39:31Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces the DataModel class for v0.9 of the web renderer, which provides a reactive data store with a subscription mechanism. The implementation is well-tested and includes comprehensive notification logic for subscribers. I've provided a couple of suggestions for improvement: one to prevent corrupting array data structures and another for enhancing performance in scenarios with many subscriptions. Overall, this is a solid implementation of a core feature.

renderers/web_core/src/v0_9/state/data-model.ts

jacobsimionato · 2026-02-09T02:54:00Z

I have addressed the feedback. Please take another look.

jacobsimionato · 2026-02-09T02:54:11Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces the DataModel for v0.9 of the web renderers, which is a core component for managing client-side state. The implementation is well-structured and comes with a comprehensive test suite, which is great to see. The related documentation and dependency updates are also included.

My review has identified a couple of areas for improvement. The most significant is that the path parsing logic in DataModel is not fully compliant with the JSON Pointer (RFC 6901) specification, which could lead to issues with certain property keys. I've also noted a potential performance optimization in the subscriber notification logic. Lastly, please remember to fill out the Pre-Review Checklist in the pull request description as per the repository's contribution guidelines.

renderers/web_core/src/v0_9/state/data-model.ts

gspencergoog · 2026-02-09T16:49:39Z

specification/v0_9/docs/a2ui_protocol.md

 - **Scope instantiation:** For every item in the array, the client instantiates the template component.
 - **Relative resolution:** Inside these instantiated components, any path that **does not** start with a forward slash `/` is treated as a **Relative Path**.
  - A relative path `firstName` inside a template iterating over `/users` resolves to `/users/0/firstName` for the first item, `/users/1/firstName` for the second, etc.
+  - It is an error to use a non-numeric index on a path segment that refers to an array.


This is a good addition.

specification/v0_9/docs/a2ui_protocol.md

ditman

Small question about the creation of Subscription objects!

renderers/web_core/src/v0_9/state/data-model.ts

ditman

Some questions about the new DataModel object, I think the only really critical thing is the regexp to check if the last segment is digits only being repeated 3x, it should be a helper function.

Also, some questions about the internal data storage type.

ditman · 2026-02-18T18:36:51Z

renderers/web_core/src/v0_9/state/data-model.ts

+   * - For objects: Setting a property to `undefined` removes the key from the object.
+   * - For arrays: Setting an index to `undefined` sets that index to `undefined` but preserves the array length (sparse array).
+   */
+  set(path: string, value: any): void {


I think it'd be nice if this object was more like an "Observable Map", IMO this should return this, see Map.prototype.set(), for example: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/set#return_value

I like this suggestion, let's do this.

ditman · 2026-02-18T18:45:12Z

renderers/web_core/src/v0_9/state/data-model.ts

+export class DataModel {
+  private data: any = {};


There's a lot of any in this class. can we say data must be a Record or Map or similar, instead of any?

I think Record<string, unknown> seems to be the best fit, because the structure of the map is unknown.

ditman · 2026-02-18T18:45:52Z

renderers/web_core/src/v0_9/state/data-model.ts

+ */
+export class DataModel {
+  private data: any = {};
+  private readonly subscriptions: Map<string, Set<SubscriptionImpl<any>>> = new Map();


Suggested change

private readonly subscriptions: Map<string, Set<SubscriptionImpl<any>>> = new Map();

private readonly subscriptions: Map<string, Set<Subscription<any>>> = new Map();

We should only use the Impl to create new instances, but in general we should code against the interface, if we can!

Here, we need a reference to the Impl because it exposes the 'setValue' method which data-model needs access to e.g. on line 213. But it only exposes these objects via the more narrow Subscription interface publicly.

ditman · 2026-02-18T18:47:29Z

renderers/web_core/src/v0_9/state/data-model.ts

+
+  /**
+   * Updates the model at the specific path and notifies all relevant subscribers.
+   * If path is '/' or empty, replaces the entire root.


What happens if path is null or undefined? Does that become a runtime error?

Path can't be null or undefined due to the magic of TS, right?

ditman · 2026-02-18T18:49:50Z

renderers/web_core/src/v0_9/state/data-model.ts

+      current = current[segment];
+    }
+
+    if (Array.isArray(current) && !/^\d+$/.test(lastSegment)) {


This regexp is repeated here and in line 107 and 95, please create a helper "isOnlyNumbers" function or similar!

Sounds great

ditman · 2026-02-18T18:54:14Z

renderers/web_core/src/v0_9/state/data-model.ts

+ * A standalone, observable data store representing the client-side state.
+ * It handles JSON Pointer path resolution and subscription management.
+ */
+export class DataModel {


Is there any way of retrieving the whole built data object?

Yes! Call myDataModel.get('/').

ditman · 2026-02-18T18:57:56Z

renderers/web_core/src/v0_9/state/data-model.ts

+  private parsePath(path: string): string[] {
+    return path.split('/').filter(p => p.length > 0);
+  }


Are there any restrictions on what the syntax for a path can be, other than "separated by /"?

Hmmm I'm not sure. I don't think we were super clear in the spec. I think as time goes on, we'll likely be more specific about this in the spec and then can add corresponding validation here.

jacobsimionato added 2 commits January 30, 2026 08:28

Add gemignore

6ab973e

Add data model

d32c5e3

github-project-automation bot moved this to Todo in A2UI Feb 9, 2026

github-project-automation bot added this to A2UI Feb 9, 2026

gemini-code-assist bot reviewed Feb 9, 2026

View reviewed changes

renderers/web_core/src/v0_9/state/data-model.ts Show resolved Hide resolved

renderers/web_core/src/v0_9/state/data-model.ts Show resolved Hide resolved

renderers/web_core/src/v0_9/state/data-model.ts Show resolved Hide resolved

jacobsimionato added 3 commits February 9, 2026 12:15

Merge branch 'main' into implement-data-model

2373b3c

Try to fix CI

a473089

fix: Address PR feedback

1e2e678

gemini-code-assist bot reviewed Feb 9, 2026

View reviewed changes

jacobsimionato added 2 commits February 9, 2026 13:01

Revert standard catalog change

2fc9919

fix: Address PR feedback

758aa1a

gemini-code-assist bot reviewed Feb 9, 2026

View reviewed changes

renderers/web_core/src/v0_9/state/data-model.ts Show resolved Hide resolved

renderers/web_core/src/v0_9/state/data-model.ts Show resolved Hide resolved

fix: Address PR feedback

4d6c1df

gemini-code-assist bot reviewed Feb 9, 2026

View reviewed changes

renderers/web_core/src/v0_9/state/data-model.ts Show resolved Hide resolved

renderers/web_core/src/v0_9/state/data-model.ts Show resolved Hide resolved

Fix standard catalog def

89ebdc6

jacobsimionato requested review from ditman and gspencergoog February 9, 2026 04:46

gspencergoog approved these changes Feb 9, 2026

View reviewed changes

ditman reviewed Feb 9, 2026

View reviewed changes

renderers/web_core/src/v0_9/state/data-model.ts Outdated Show resolved Hide resolved

jacobsimionato requested a review from paullewis February 16, 2026 04:32

jacobsimionato added 2 commits February 18, 2026 14:08

Fix docs

e4b3e7b

Update subscription model

d50c929

jacobsimionato requested a review from ava-cassiopeia as a code owner February 18, 2026 04:00

ditman reviewed Feb 18, 2026

View reviewed changes

-});
+  it('overwrites primitive values in path when setting nested properties', () => {
+    model.set('/user/name', 'not an object');
+    assert.strictEqual(model.get('/user/name'), 'not an object');
+    // Now, try to set a nested property. This should overwrite the primitive.
+    model.set('/user/name/first', 'Alice');
+    assert.deepStrictEqual(model.get('/user/name'), { first: 'Alice' });
+    assert.strictEqual(model.get('/user/name/first'), 'Alice');
+  });
+});

	private readonly subscriptions: Map<string, Set<SubscriptionImpl<any>>> = new Map();
	private readonly subscriptions: Map<string, Set<Subscription<any>>> = new Map();

Conversation

jacobsimionato commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobsimionato commented Feb 9, 2026

Uh oh!

jacobsimionato commented Feb 9, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacobsimionato commented Feb 9, 2026

Uh oh!

jacobsimionato commented Feb 9, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

jacobsimionato commented Feb 9, 2026

Uh oh!

jacobsimionato commented Feb 9, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jacobsimionato commented Feb 9, 2026 •

edited

Loading