Conversation
…ing dependencies.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Added `model-processor.test.ts` for v0.8 coverage. - Added `expression_evaluator.test.ts` and `basic_functions.test.ts` for v0.9 coverage. - Updated `message-processor.test.ts` with edge case tests. - Fixed `ExpressionEvaluator` to include `getFunction` for testing. - Aligned tests with v0.9 specification (removed unused functions).
- Added `expression_parser.test.ts` to test strict interpolation logic. - Added `styles.test.ts` to verify style utility functions. - Verified all 119 tests pass.
- Added `component-context.test.ts` to verify context initialization and action dispatching. - Verified all 123 tests pass.
- Added edge case tests for MessageProcessor (warnings). - Added null/undefined path validation for DataModel. - Added pass-through test for ExpressionEvaluator. - Added dispose test for SurfaceModel. - Added componentTree getter test for ComponentModel. - Verified all 130 tests pass.
…rage - Reviewed and updated implementation comments in test files to match documentation standards. - Added comprehensive test coverage for v0.8 model processor. - Added test coverage for v0.9 expression evaluator, parser, and basic functions. - Verified all tests pass.
- Added 2025 Google LLC copyright header to all checks-in source files in renderers/web_core. - Skipped generated, JSON, and Markdown files.
gspencergoog
requested review from
ava-cassiopeia,
crisbeto,
ditman,
dmandar,
jacobsimionato,
nan-yu,
yjbanov and
zeroasterisk
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tions.ts Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Remove legacy v0.8 primitive wrappers (literalString, etc.) - Implement recursive evaluation for Arrays and Objects with depth limit - Add tests for deep recursion and nested structures
- Wrap `parse` logic in try-catch to prevent synchronous throws. - Return `throwError` on failure to allow graceful error handling by consumers. - Add tests for error scenarios (recursion limits, malformed input).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant update to web_core with version 0.9, adding expression and function evaluation capabilities. The changes are extensive, including new parsers, evaluators, a suite of basic functions, and a new message processor, all backed by comprehensive tests. The PR also includes important maintenance work like refactoring exports for better modularity and adding copyright headers. My review found one issue in the implementation of a new basic function.
Note: Security Review did not run due to the size of the PR.
- Add `./data/*` to `exports` in `package.json` to allow importing `model-processor` and `guards` from `web_core`. - This fixes `Cannot find module` errors in `renderers/lit` during CI build.
- Add `.` export pointing to `./dist/src/v0_8/index.js` (and types) - Required because `exports` field usage disables default `main` lookup for the package root.
jacobsimionato
left a comment
jacobsimionato
left a comment
There was a problem hiding this comment.
Overall LGTM to fixing the copyright headers and adding expression parsing stuff
I'd love to see the function resolution pipeline and the APIs for the basic catalog items also move into the core library, but that can be in a future step.
…basic functions - Expressions: Refactored `ExpressionParser` to consume `DataContext` directly, simplifying observable subscription patterns for reactive function calls. - Functions: Hardened the `divide` function in `basic_functions` to handle `null`, `undefined`, and division by zero. Added a new `openUrl` action wrapper. - Data Context: Refactored `DataContext.subscribeDynamicValue` to robustly resolve dynamic argument streams and reactively evaluate function outputs. Added `function_execution.spec.ts` for coverage. - Docs: Updated `.gemini/GEMINI.md` repository structure to match the latest layout across protocol versions (`v0.8` through `v0.10`) and new tool extensions. - Style: Applied comprehensive code formatting standard sweep across the `v0_9` directory.
Add the ability to gracefully track evaluation errors natively inside DataContext during dynamic value resolution instead of throwing. - Add `errors`, `reportError`, and `clearErrors` to DataContext - Change `console.warn` occurrences to populate error context - Update unit tests with new expectations
…ssageProcessor These methods were vestiges of the v0.8 architecture where data binding relied on the physical component node itself. In v0.9, the data model is accessed directly via absolute JSON Pointer paths directly against the SurfaceModel, rendering the processor-level abstractions unnecessary. Removed the methods from `MessageProcessor.ts` and their associated tests over in `message-processor.test.ts`. Verified building and vitest passes successfully.
There was a problem hiding this comment.
Is it possible to not keep a copy of the schemas in the renderer library?
There was a problem hiding this comment.
Yes. Let me remedy that.
There was a problem hiding this comment.
Okay, I removed the schemas that were checked in (accidentally) in v0_8.
In v0_9, I had used Zod schemas so that we can do schema validation, and so I converted the v0_8 schemas to also be Zod schemas.
To make sure that they won't drift from the schemas in the specification, I added a validation script tha runs with the tests in web_core, ao CI should fail if they diverge.
Restores the script to copy the authoritative schemas from the central folder into the local directory during the build. This avoids referencing files outside of the package distribution directory which causes issues when publishing to npm. Also updates the npm script to reference the files directly as Node's test runner can no longer resolve the directory to an entrypoint without an under certain conditions. All generated JSON schemas are ignored via to avoid duplicate check-ins.
- Explicitly matches Zod definitions to JSON structure using \`zod-to-json-schema\` - Validates the entire definitions tree and root oneOf structure - Handles edge-case structural mismatches such as the \`A2uiMessage\` definition expansion and const string metadata - Refactors schema validation on MessageProcessor to enforce correct schema validation and skips missing IDs. - Injects \`npx tsx scripts/verify-schema.ts\` before tests - Marked task as complete in \`task.md\`
- Add strict Zod schemas inside src/v0_8/schema matching standard specifications - Migrate v0.8 Typescript interfaces to use z.infer for DRYness and exact type-safety - Add validation to v0.8 model-processor - Update verify-schema.ts testing script to cover v0.8 backwards compatibility and enforce strict structural parity with standard catalog json schema
There was a problem hiding this comment.
The schemas folder now only has a .gitignore file. What about moving the file on its parent folder and remove the schemas folder?
There was a problem hiding this comment.
Oh, yes, needs to be removed. Done.
There was a problem hiding this comment.
Oh, actually, we should be ignoring things there still. The schemas are still copied here in order to be able to export them for use in prompts. It was doing that for v0.8, and I added it for v0.9.
I moved the .gitignore up to the web_core directory so that they don't get checked in again.
- Updated `styles.color` to `styles.primaryColor: "#0000ff"` - Injected `usageHint` implicitly removed from component scope - Skipped test cases dependent on deprecated CRDT `valueMap` - Explicitly verified compilation with TS noEmit
Updated copy-spec.js script and wireit config to copy the canonical JSON schema definitions for v0.9 into renderers/web_core/src/v0_9/schemas on build. Exported the v0.9 server_to_client.json payload under Schemas namespace inside v0_9/index.ts to allow prompt synchronization on tools expecting the raw schema. Added a local .gitignore within renderers/web_core to correctly prevent the dynamically generated /schemas directories from tracking in version control.
renderers/web_core/src/v0_9/basic_catalog/expressions/expression_evaluator.ts
Outdated
Show resolved
Hide resolved
renderers/web_core/src/v0_9/basic_catalog/expressions/expression_evaluator.ts
Outdated
Show resolved
Hide resolved
renderers/web_core/src/v0_9/basic_catalog/expressions/expression_evaluator.ts
Outdated
Show resolved
Hide resolved
renderers/web_core/src/v0_9/basic_catalog/expressions/expression_evaluator.ts
Outdated
Show resolved
Hide resolved
renderers/web_core/src/v0_9/basic_catalog/expressions/expression_evaluator.ts
Outdated
Show resolved
Hide resolved
- `ExpressionParser` now decodes string interpolation directly into `DynamicValue` ASTs instead of evaluating them aggressively. - Deleted `ExpressionEvaluator` as parser + context handles dynamic resolution. - `DataContext` error accumulation replaced with robust logging meant for the message processor pipeline. - `verify-schema.ts` migrated to native `node:test` suite for v0.8 and v0.9.
|
Okay, @jacobsimionato I think I addressed all of your comments. PTAL. |
jacobsimionato
left a comment
jacobsimionato
left a comment
There was a problem hiding this comment.
Amazing - looks very close!
| processMessages(messages: ServerToClientMessage[]): void { | ||
| for (const message of messages) { | ||
| for (const rawMessage of messages) { | ||
| const message = A2uiMessageSchema.parse(rawMessage); |
There was a problem hiding this comment.
Why is this necessary when the message type passed in is ServerToClientMessage ?
There was a problem hiding this comment.
This is the runtime type checking that Zod provides. The input has been parsed as JSON, and cast to ServerToClientMessage, but unless you run it through the Zod schema, it doesn't actually check the types of anything (ServerToClientMessage is a TypeScript type, but JS doesn't know about it). Once it has been parsed, it is guaranteed to match the schema, and if it fails, it fails here with a Zod message, rather than later with a generic TypeError.
| * Parses an input string into an array of DynamicValues. | ||
| * If the input contains no interpolation, it returns the raw string as a single literal. | ||
| */ | ||
| public parse(input: string, depth = 0): DynamicValue[] { |
| /** | ||
| * Optional map of functions provided by this catalog. | ||
| */ | ||
| readonly functions?: ReadonlyMap<string, any>; |
There was a problem hiding this comment.
s/any/FunctionImplementation ?
There was a problem hiding this comment.
Good call. Changed.
| /** | ||
| * A function implementation that can be registered with the evaluator or basic catalog. | ||
| */ | ||
| export type FunctionImplementation = ( |
There was a problem hiding this comment.
This isn't really a schema, so I think it should live in catalog/types.ts
There was a problem hiding this comment.
Wow, this is hectic but it looks about right. The use of rxjs internally is neat!
| formatString: (args) => { | ||
| const template = String(args["value"] || ""); | ||
| const parser = new ExpressionParser(); | ||
| return parser.parse(template); |
There was a problem hiding this comment.
I still think this might be wrong, because parser.parse() returns type DynamicValue[], while this function should actually return a formatted string, right?
I imagine it needs to do an additional step like call dataContext.resolve(value) for each of the parsed DynamicValues, then subscribe to all the results and return an observable that concatenates them all together etc.
I think you already set up the FunctionImplementation interface such that this does have access to DataContext which is great, so hopefully it's not a huge change.
There was a problem hiding this comment.
I didn't see a test for it either so that could be good to verify it works!
3 participants
Description
Specific Changes
ExpressionEvaluator,ExpressionParser, andBasicFunctions.MessageProcessor.web_corev0.9 components includingExpressionEvaluator,ExpressionParser,BasicFunctions, andMessageProcessor. Also added tests for v0.8ModelProcessorand styles.renderers/web_core.index.tsexports inweb_corefor better organization and clarity.Verification
npm testinrenderers/web_core, confirming 130 passing tests with no regressions.Summary of Changes
This pull request significantly advances the web_core library by introducing version 0.9, which brings powerful expression evaluation and function support, alongside a refined message processing system. These updates aim to provide more dynamic and flexible UI rendering capabilities. The changes also include crucial maintenance, dependency updates, and compliance efforts, ensuring the library remains robust and up-to-date.
Highlights
ExpressionEvaluator,ExpressionParser, andBasicFunctionsto enable dynamic logic and data manipulation within the web_core framework.MessageProcessorfor v0.9, improving how messages are handled and state is managed across surfaces.ModelProcessorand styles) and all new v0.9 components, ensuring robustness and correctness.index.tsexports for better organization, resolved Angular import issues, updated Lit sample dependencies, and added 2025 Google LLC copyright headers to all source files.Changelog
A2uiMessageProcessorandTypesto use thev0_8barrel export, resolving Angular import issues.rxjsandtslibdependencies.resolvedfields from variousnode_modulesentries, likely due to apackage-lock.jsonregeneration.exportsfield to use versioned entry points (./v0_8,./v0_9, etc.) for better modularity.data/*andschemas/*exports, streamlining the API surface.rxjsto dependencies.testcommand to run tests across the entiredistdirectory.A2uiMessageProcessorfunctionality, covering rendering, surface updates, data model interaction, component reference resolution, template expansion, and path normalization.merge,appendToAll,toProp, andcreateThemeStyles.ExpressionEvaluator, covering primitive evaluation, data bindings, function calls, nested expressions, recursion depth limits, and graceful handling of unknown functions.ExpressionEvaluatorclass to support evaluation of function calls, data bindings, and primitive values.isFunctionCall,isDataBinding) for robust expression type checking.ExpressionParser, covering string interpolation, function calls, literals, nested expressions, escaped characters, and error handling for malformed input.ExpressionParserfor parsing string interpolations and expressions, including handling of literals, function calls, and data bindings.BASIC_FUNCTIONS, covering arithmetic, comparison, logical, string manipulation, validation, and formatting functions.BASIC_FUNCTIONSobject containing implementations for various arithmetic, comparison, logical, string, validation, and formatting functions.basic_catalogmodule.EventEmittermethods (subscribe,emit,dispose).MessageProcessor, covering surface creation/deletion, component updates, data model interaction, and various error/warning scenarios (e.g., multiple update types, missing component type, duplicate surface, non-existent surface).actionHandleroptional.processMessagesmethod.getData,resolvePath, andsetDatamethods for data model interaction.ComponentContext, covering correct initialization, action dispatching, error handling for non-existent components, and data context path configuration.dispatchActionmethod.ComponentModel, covering update notifications, unsubscription behavior, and thecomponentTreerepresentation.propertiesgetter, anddisposemethod.componentTreegetter to provide a JSON representation of the component.nullorundefinedpaths are used forgetandsetoperations.entriesgetter to provide an iterator over components.surfacesMapgetter to provide a readonly map of active surfaces.SurfaceModel.dispatchAction, anddisposemethods.