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 all 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
41 changes: 28 additions & 13 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 ValueNotifier<UiDefinition?> notifier = getSurfaceNotifier(
surfaceId,
);
final isNew = notifier.value == null;

// Caching logic remains the same
UiDefinition uiDefinition =
notifier.value ?? UiDefinition(surfaceId: surfaceId);
final Map<String, Component> newComponents = Map.of(
Expand All @@ -176,26 +174,34 @@ class GenUiManager implements GenUiHost {
}
uiDefinition = uiDefinition.copyWith(components: newComponents);
notifier.value = uiDefinition;
if (isNew) {
genUiLogger.info('Adding surface $surfaceId');
_surfaceUpdates.add(SurfaceAdded(surfaceId, uiDefinition));
} else {

// Notify UI ONLY if rendering has begun (i.e., rootComponentId is set)
if (uiDefinition.rootComponentId != null) {
genUiLogger.info('Updating surface $surfaceId');
_surfaceUpdates.add(SurfaceUpdated(surfaceId, uiDefinition));
} else {
genUiLogger.info(
'Caching components for surface $surfaceId (pre-rendering)',
);
}
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));

// ALWAYS fire SurfaceAdded, as this is the signal to start rendering.
genUiLogger.info('Creating and rendering surface $surfaceId');
_surfaceUpdates.add(SurfaceAdded(surfaceId, newUiDefinition));
case DataModelUpdate():
final String path = message.path ?? '/';
genUiLogger.info(
Expand All @@ -205,6 +211,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
32 changes: 13 additions & 19 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 @@ -41,24 +41,19 @@ void main() {
),
];

final Future<GenUiUpdate> futureAdded = manager.surfaceUpdates.first;
manager.handleMessage(
SurfaceUpdate(surfaceId: surfaceId, components: components),
);
final GenUiUpdate addedUpdate = await futureAdded;
expect(addedUpdate, isA<SurfaceAdded>());
expect(addedUpdate.surfaceId, surfaceId);

final Future<GenUiUpdate> futureUpdated = manager.surfaceUpdates.first;
final Future<GenUiUpdate> futureUpdate = manager.surfaceUpdates.first;
manager.handleMessage(
const BeginRendering(surfaceId: surfaceId, root: 'root'),
);
final GenUiUpdate updatedUpdate = await futureUpdated;
final GenUiUpdate update = await futureUpdate;

expect(updatedUpdate, isA<SurfaceUpdated>());
expect(updatedUpdate.surfaceId, surfaceId);
final UiDefinition definition =
(updatedUpdate as SurfaceUpdated).definition;
expect(update, isA<SurfaceAdded>());
expect(update.surfaceId, surfaceId);
final UiDefinition definition = (update as SurfaceAdded).definition;
expect(definition, isNotNull);
expect(definition.rootComponentId, 'root');
expect(manager.surfaces[surfaceId]!.value, isNotNull);
Expand Down Expand Up @@ -90,18 +85,17 @@ void main() {
),
];

final Future<GenUiUpdate> futureUpdate = manager.surfaceUpdates.first;
expectLater(
manager.surfaceUpdates,
emitsInOrder([isA<SurfaceAdded>(), isA<SurfaceUpdated>()]),
);
Comment on lines +88 to +91
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 test correctly verifies the order of events, but it no longer checks the content of the SurfaceUpdated event as the previous version of the test did. It would be more robust to add a check to ensure the components were actually updated, for example by using the having matcher.

Suggested change
expectLater(
manager.surfaceUpdates,
emitsInOrder([isA<SurfaceAdded>(), isA<SurfaceUpdated>()]),
);
expectLater(
manager.surfaceUpdates,
emitsInOrder([
isA<SurfaceAdded>(),
isA<SurfaceUpdated>().having(
(e) => e.definition.components['root'],
'updated component',
newComponents[0],
),
]),
);


manager.handleMessage(
const BeginRendering(surfaceId: surfaceId, root: 'root'),
);
manager.handleMessage(
SurfaceUpdate(surfaceId: surfaceId, components: newComponents),
);
final GenUiUpdate update = await futureUpdate;

expect(update, isA<SurfaceUpdated>());
expect(update.surfaceId, surfaceId);
final UiDefinition updatedDefinition =
(update as SurfaceUpdated).definition;
expect(updatedDefinition.components['root'], newComponents[0]);
expect(manager.surfaces[surfaceId]!.value, updatedDefinition);
},
);

Expand Down
5 changes: 4 additions & 1 deletion packages/genui/test/ui_tools_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ void main() {
);

await tool.invoke(args);
genUiManager.handleMessage(
const BeginRendering(surfaceId: 'testSurface', root: 'root'),
);

await future;
});
Expand Down Expand Up @@ -99,7 +102,7 @@ void main() {
final Future<void> future = expectLater(
genUiManager.surfaceUpdates,
emits(
isA<SurfaceUpdated>()
isA<SurfaceAdded>()
.having((e) => e.surfaceId, surfaceIdKey, 'testSurface')
.having(
(e) => e.definition.rootComponentId,
Expand Down
Loading