Fix surface creation and update notification bugs

[jacobsimionato]

This PR addresses several issues related to surface creation and update notifications in the genui package.

Changes:

• Corrected the logic in GenUiManager to ensure that SurfaceAdded events are fired only when a new surface is created, and SurfaceUpdated events are fired for all subsequent modifications, including when rendering begins.
• Updated the genui_manager_test.dart to accurately reflect the corrected event firing logic, ensuring that the tests properly validate the behavior of GenUiManager.
• Applied automated fixes and formatting across the codebase.

[jacobsimionato]

/gemini review

[gemini-code-assist]

Code Review

This pull request correctly refactors the logic for surface creation and update notifications. The distinction between new and existing surfaces is now handled more robustly in the SurfaceUpdate case, and DataModelUpdate now correctly triggers a UI refresh for rendered surfaces. The tests have also been updated to reflect these changes. I've added one comment to improve the robustness of the BeginRendering message handler to align it with the new logic for handling new surfaces.

[gemini-code-assist]

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));
        }

[gemini-code-assist]

Code Review

This pull request correctly refactors the logic for surface creation and update notifications, ensuring SurfaceAdded is fired for new surfaces and SurfaceUpdated for modifications. The changes in GenUiManager and the corresponding test updates in genui_manager_test.dart are well-aligned with this goal.

I've identified a couple of areas for improvement. One is a potential memory leak and unintended side effect in the DataModelUpdate handler. The other is an inconsistency in the BeginRendering handler, which doesn't correctly fire a SurfaceAdded event for new surfaces. Addressing these points will improve the robustness and correctness of the GenUiManager.

[gemini-code-assist]

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));
        }

[gemini-code-assist]

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));
	}
	}

[jacobsimionato]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively addresses the bugs in surface creation and update notifications by correctly assigning the SurfaceAdded event to the BeginRendering message and ensuring SurfaceUpdated is used for subsequent modifications. The accompanying test updates properly validate this new logic. I've included a couple of suggestions to enhance code conciseness and improve the robustness of one of the tests.

[gemini-code-assist]

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));
	}

[gemini-code-assist]

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],
	),
	]),
	);