fix(v0.9): make createSurface idempotent + fix React README quick-start#1225
fix(v0.9): make createSurface idempotent + fix React README quick-start#1225andrewkolos wants to merge 6 commits intogoogle:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request makes the createSurface operation idempotent in the MessageProcessor, updating the implementation, tests, and documentation accordingly. The React README example is also expanded to show a more complete message processing flow. Feedback focuses on improving the React example to handle existing surfaces when components remount and moving the idempotency check in the core logic to better handle malformed retry attempts.
bd9b434 to
3deb0ee
Compare
|
I'm going to address each issue in separate PRs since I'm conflating problems here. |
|
Reopening. I had to think about apps using |
|
Side note from testing: |
d031c1a to
0e1b913
Compare
|
|
||
| if (this.model.getSurface(surfaceId)) { | ||
| throw new A2uiStateError(`Surface ${surfaceId} already exists.`); | ||
| return; |
There was a problem hiding this comment.
I'm not sure about this, I think this is a feature, not a bug, I'd follow up with @jacobsimionato here. However, in case we want to hide the surface duplication, we should at least console.warn so users are aware that maybe their agent is misbehaving, instead of hiding this silently.
There was a problem hiding this comment.
AFAICT Jacob already blessed this approach in #1186: #1186 (comment)
Re: console.warn: in the React StrictMode case (which is what the quick-start hits), a warning would fire on every dev-mode render, adding noise. The React strict mode It could be avoided but it would need additional "already-initialized?" guard logic that would bloat the example code more than it already is. Even with that adjustment, the error would appear even more often when #1186 lands barring some major overhaul.
In the interest of getting to a state where the quick start example it runs, I will change this to console.warn anyway.
There was a problem hiding this comment.
Re: console.warn: in the React StrictMode case (which is what the quick-start hits), a warning would fire on every dev-mode render, adding noise. The React strict mode It could be avoided but it would need additional "already-initialized?" guard logic that would bloat the example code more than it already is.
I think this is all right, the browser console should collapse two console entries that are identical:
Even with that adjustment, the error would appear even more often when #1186 lands barring some major overhaul.
Yeah, the sample code needs some love; in my case it's duplicating every protocol message, and I'm not sure if that's expected
AFAICT Jacob already blessed this approach in #1186: #1186 (comment)
Hmmm, I'm not sure he approves of this approach in that text. The protocol is not very clear, but it seems to indicate that duplicate createSurfaces are an error. Also, what'd be the behavior? IMO we should not drop the newer messages, but instead overwrite the previously existing ones maybe?
There was a problem hiding this comment.
Hey! Sorry in my previous PR I was trying all kinds of hacks to resolve this, but I never fully understood the underlying problem here of the useEffect behavior. Thank you for drilling to the root issue!
I agree with @ditman that the renderer library should fail here. We discussed an intention for duplicate createSurface messages with the same surfaceId to fail, partly to cover a collision scenario where two independent subagents try to create a surface with the same ID - one of them should fail. But I see that this never made it into the specification. I'll send a PR to clarify this.
Re how to deal with this issue practically, I'm not a React expert, but I think that we should rearchitect the system as necessary to avoid message duplication. I believe that useEffect has this behavior in dev because it's intended to be used to drive idempotent actions, e.g. rendering. But A2UI messages are not all idempotent, so I guess useEffect is not the right way to add placeholder content or convey messages from the transport layer to the MessageProcessor. Could we resolve this by avoiding ever calling messageProcessor.processMessage() or messageProcessor.processMessages() etc from useEffect?
Apologies that I likely caused this issue to emerge by relying on useEffect in the wrong place! I really appreciate having your real React expertise here to fix this kind of stuff.
ditman
left a comment
There was a problem hiding this comment.
I think the throwiness of the message-processor is intended! /cc @jacobsimionato for a 2nd opinion :)
processCreateSurfaceMessage threw when a surface with the same ID already existed. This crashes in three real-world scenarios: 1. React StrictMode double-fires useEffect, creating the surface twice. The published README quick-start hits this immediately. 2. A2A streaming: status-update events carry cumulative message parts, redelivering createSurface on every chunk. 3. ADK conversation history replay: the full history is sent with each new request, including prior createSurface messages. The spec says "once a surface is created, its surfaceId and catalogId are fixed; to reconfigure them, the surface must be deleted and recreated." A duplicate createSurface is a no-op by spec intent. Change the throw to a silent return. Update the existing test that asserted the throw to instead assert the no-op behavior.
036511a to
e24d3f0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the MessageProcessor to log a warning instead of throwing an error when a duplicate createSurface message is encountered. It also enhances the React renderer's README with a more complete example of the A2UI protocol and includes relevant test updates and a changelog entry. I have no feedback to provide.
|
I've decided to supersede this with a new PR, as it drops the "make createSurface idempotent" idea: #1250 |
Fixes #1223. Fixes #1224. Bug descriptions and repro steps can be found on the issues.
Also augments the quick-start example with some messages so that the user actually seeing something instead of [Loading root...] forever.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.