Skip to content

Fix surface creation and update notification bugs #556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix surface creation and update notification bugs #556

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c52051e
Fix bug with surface creation and update notifications
jacobsimionato Nov 16, 2025
2624167
Apply automated fixes and formatting
jacobsimionato Nov 19, 2025
eae2d4c
Fix(tests): Correct event stream handling in GenUiManager tests
jacobsimionato Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions packages/genui/lib/src/core/genui_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,12 @@ class GenUiManager implements GenUiHost {
void handleMessage(A2uiMessage message) {
switch (message) {
case SurfaceUpdate():
// No need for SurfaceAdded here because A2uiMessage will never generate
// those. We decide here if the surface is new or not, and generate a
// SurfaceAdded event if so.
final String surfaceId = message.surfaceId;
final bool isNewSurface = !_surfaces.containsKey(surfaceId);
final ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
surfaceId,
);
final isNew = notifier.value == null;

UiDefinition uiDefinition =
notifier.value ?? UiDefinition(surfaceId: surfaceId);
final Map<String, Component> newComponents = Map.of(
Expand All @@ -176,26 +174,31 @@ class GenUiManager implements GenUiHost {
}
uiDefinition = uiDefinition.copyWith(components: newComponents);
notifier.value = uiDefinition;
if (isNew) {
genUiLogger.info('Adding surface $surfaceId');

if (isNewSurface) {
genUiLogger.info('Adding new surface $surfaceId');
_surfaceUpdates.add(SurfaceAdded(surfaceId, uiDefinition));
} else {
genUiLogger.info('Updating surface $surfaceId');
_surfaceUpdates.add(SurfaceUpdated(surfaceId, uiDefinition));
}
case BeginRendering():
dataModelForSurface(message.surfaceId);
final String surfaceId = message.surfaceId;
dataModelForSurface(surfaceId);
final ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
message.surfaceId,
surfaceId,
);

// Update the definition with the root component
final UiDefinition uiDefinition =
notifier.value ?? UiDefinition(surfaceId: message.surfaceId);
notifier.value ?? UiDefinition(surfaceId: surfaceId);
final UiDefinition newUiDefinition = uiDefinition.copyWith(
rootComponentId: message.root,
);
notifier.value = newUiDefinition;
genUiLogger.info('Started rendering ${message.surfaceId}');
_surfaceUpdates.add(SurfaceUpdated(message.surfaceId, newUiDefinition));

genUiLogger.info('Start rendering surface $surfaceId');
_surfaceUpdates.add(SurfaceUpdated(surfaceId, newUiDefinition));
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

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

high

The current implementation for BeginRendering assumes that a SurfaceUpdate message has already been received for the given surfaceId. If BeginRendering is the first message for a new surface, it will be created, but a SurfaceUpdated event will be fired instead of SurfaceAdded.

This is inconsistent with the logic in the SurfaceUpdate case, which correctly handles new surfaces by firing SurfaceAdded.

To make this more robust and handle the edge case where BeginRendering creates the surface, consider checking if the surface is new and firing the appropriate event, similar to how it's done in the SurfaceUpdate case.

        final String surfaceId = message.surfaceId;
        final bool isNewSurface = !_surfaces.containsKey(surfaceId);
        dataModelForSurface(surfaceId);
        final ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
          surfaceId,
        );

        // Update the definition with the root component
        final UiDefinition uiDefinition =
            notifier.value ?? UiDefinition(surfaceId: surfaceId);
        final UiDefinition newUiDefinition = uiDefinition.copyWith(
          rootComponentId: message.root,
        );
        notifier.value = newUiDefinition;

        genUiLogger.info('Start rendering surface $surfaceId');
        if (isNewSurface) {
          _surfaceUpdates.add(SurfaceAdded(surfaceId, newUiDefinition));
        } else {
          _surfaceUpdates.add(SurfaceUpdated(surfaceId, newUiDefinition));
        }

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

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

high

The current implementation for BeginRendering always fires a SurfaceUpdated event. This is inconsistent with the logic for SurfaceUpdate and can lead to issues on the client side if BeginRendering is the first message for a given surface. In that case, the client would receive a SurfaceUpdated event for a surface it hasn't seen before, instead of the expected SurfaceAdded.

To fix this, you should check if the surface is new before getting the notifier, and then fire SurfaceAdded or SurfaceUpdated accordingly, just like you've done for the SurfaceUpdate case.

        final String surfaceId = message.surfaceId;
        final bool isNewSurface = !_surfaces.containsKey(surfaceId);
        dataModelForSurface(surfaceId);
        final ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
          surfaceId,
        );

        // Update the definition with the root component
        final UiDefinition uiDefinition =
            notifier.value ?? UiDefinition(surfaceId: surfaceId);
        final UiDefinition newUiDefinition = uiDefinition.copyWith(
          rootComponentId: message.root,
        );
        notifier.value = newUiDefinition;

        if (isNewSurface) {
          genUiLogger.info('Adding and rendering new surface $surfaceId');
          _surfaceUpdates.add(SurfaceAdded(surfaceId, newUiDefinition));
        } else {
          genUiLogger.info('Start rendering surface $surfaceId');
          _surfaceUpdates.add(SurfaceUpdated(surfaceId, newUiDefinition));
        }

case DataModelUpdate():
final String path = message.path ?? '/';
genUiLogger.info(
Expand All @@ -205,6 +208,15 @@ class GenUiManager implements GenUiHost {
);
final DataModel dataModel = dataModelForSurface(message.surfaceId);
dataModel.update(DataPath(path), message.contents);

// Notify UI of an update if the surface is already rendering
final ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
message.surfaceId,
);
final UiDefinition? uiDefinition = notifier.value;
if (uiDefinition != null && uiDefinition.rootComponentId != null) {
_surfaceUpdates.add(SurfaceUpdated(message.surfaceId, uiDefinition));
}
Comment on lines +216 to +222
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

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

medium

Calling getSurfaceNotifier here has an unintended side effect: if a DataModelUpdate message is received for a surface that does not exist yet, a new ValueNotifier is created and added to the _surfaces map. This could lead to a memory leak if the surface is never formally created with a SurfaceUpdate message.

To avoid this, you can directly access the _surfaces map to check for existence and retrieve the notifier without causing this side effect.

Suggested change
final ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
message.surfaceId,
);
final UiDefinition? uiDefinition = notifier.value;
if (uiDefinition != null && uiDefinition.rootComponentId != null) {
_surfaceUpdates.add(SurfaceUpdated(message.surfaceId, uiDefinition));
}
final notifier = _surfaces[message.surfaceId];
if (notifier != null) {
final uiDefinition = notifier.value;
if (uiDefinition != null && uiDefinition.rootComponentId != null) {
_surfaceUpdates.add(SurfaceUpdated(message.surfaceId, uiDefinition));
}
}

Comment on lines +215 to +222
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 19, 2025

Choose a reason for hiding this comment

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

medium

This block of code can be made more concise. You can chain the call to getSurfaceNotifier with .value and use the null-aware ?. operator for a more compact check.

Suggested change
// Notify UI of an update if the surface is already rendering
final ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
message.surfaceId,
);
final UiDefinition? uiDefinition = notifier.value;
if (uiDefinition != null && uiDefinition.rootComponentId != null) {
_surfaceUpdates.add(SurfaceUpdated(message.surfaceId, uiDefinition));
}
// Notify UI of an update if the surface is already rendering
final uiDefinition = getSurfaceNotifier(message.surfaceId).value;
if (uiDefinition?.rootComponentId != null) {
_surfaceUpdates.add(SurfaceUpdated(message.surfaceId, uiDefinition));
}

case SurfaceDeletion():
final String surfaceId = message.surfaceId;
if (_surfaces.containsKey(surfaceId)) {
Expand Down
3 changes: 3 additions & 0 deletions packages/genui/test/core/genui_manager_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ void main() {
manager.handleMessage(
SurfaceUpdate(surfaceId: surfaceId, components: oldComponents),
);
manager.handleMessage(
const BeginRendering(surfaceId: surfaceId, root: 'root'),
);

final newComponents = [
const Component(
Expand Down
Loading