Initial implementation of prompt builder.#777
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by creating a PromptBuilder to centralize and structure the generation of system prompts. The use of PromptFragments, ProtocolMessages, and SurfaceOperations makes the process more modular, maintainable, and extensible. The changes across the example apps and core library are consistent with this new approach. I've identified a few issues, including a critical syntax error that will prevent compilation, a potential regression, and some opportunities for improving maintainability and test coverage. Please see the detailed comments for specifics.
examples/travel_app/lib/src/ai_client/google_generative_ai_client.dart
Outdated
Show resolved
Hide resolved
|
|
||
| /// Code execution restriction. | ||
| /// | ||
| /// This is useful when AI may need to execute code. |
There was a problem hiding this comment.
This doesn't seem like an accurate comment.
There was a problem hiding this comment.
replaced with 'This is useful to communicate limitations of code execution to the AI.'
does it look better?
|
|
||
| String get tickedName => '`$name`'; | ||
|
|
||
| static String explainMessages(Set<ProtocolMessages> operations) { |
There was a problem hiding this comment.
This should probably be Iterable, not Set. You can call toSet() internally if you want to ensure uniqueness.
There was a problem hiding this comment.
It is convenient/ergnomic to prepare this parameter as set:
final operations = <ProtocolMessages>{};
if (create) {
operations.addAll([
ProtocolMessages.createSurface,
ProtocolMessages.updateComponents,
]);
}
if (update) {
operations.add(ProtocolMessages.updateComponents);
}
if (delete) {
operations.add(ProtocolMessages.deleteSurface);
}
if (dataModel) {
operations.add(ProtocolMessages.updateDataModel);
}
It is extra code and mental effort for reader to convert it back and forth.
Am I missing something?
There was a problem hiding this comment.
A Set is an Iterable, so you can still do that. But you can also pass a List (which is also Iterable), and there's no chance that the passed-in operations collection will be accidentally modified inside this function, since iterable can't be modified.
| required this.delete, | ||
| required this.dataModel, | ||
| }); | ||
| SurfaceOperations.createOnly({required bool dataModel}) |
There was a problem hiding this comment.
This seems overly complicated, and doesn't scale. Imagine adding 10 more message types... Why not just accept an iterable of ProtocolMessages, or even just leave it as the SurfaceOperations constructor above, and just make all the bools default to false?
There was a problem hiding this comment.
Yes, it is good idea to have all bools defaulting to false. Added.
If we have 10 operations, we will not create 10^2 constructors.
I believe though having some helper methods will add some convenience, readability and ergonomics.
There was a problem hiding this comment.
Well, but is SurfaceOperations(update: true) really more ergonomic than SurfaceOperations.updateOnly()? They both are pretty clear, and exactly the same length.
And why isn't it SurfaceOperations.update() if SurfaceOperations.createAndUpdate() exists? Ahh, because then it clashes with the member, I see. But then maybe the other should be SurfaceOperations.createAndUpdateOnly() to be consistent?
What rules will we use to know what messages to include in the "convenience" set? Even adding one more common message to the combinations would double the list (8 instead of 4 combinations), plus having createAndUpdateAndFoobar gets crazy long...
|
|
||
| parts.add(''' | ||
| IMPORTANT: | ||
| - Do not use tools or function calls for UI generation. Use JSON text blocks. |
There was a problem hiding this comment.
This might not be a great piece of advice in all cases. Some implementations might use functions to generate A2UI. For instance, if you had an MCP wrapper around it.
There was a problem hiding this comment.
defined TechnicalPossibilities
| parts.add(''' | ||
| IMPORTANT: | ||
| - Do not use tools or function calls for UI generation. Use JSON text blocks. | ||
| - Ensure all JSON is valid and fenced with ```json ... ```. |
There was a problem hiding this comment.
We should probably have a better fence than this. Something like:
<a2ui_json>
```json
```
</a2ui_json>
or
-----BEGIN_A2UI_JSON-----
```json
```
-----END_A2UI_JSON-----
There will still be JSON fences around it but we should try and still allow the generation to generate other non-A2UI JSON. Right now that isn't possible.
There was a problem hiding this comment.
-----A2UI_JSON_SCHEMA_START-----
```json
$a2uiSchema
-----A2UI_JSON_SCHEMA_END-------
|
|
||
| **OUTPUT FORMAT:** | ||
| You must output a VALID JSON object representing one of the A2UI message types ($operationsFormatted). | ||
| - Do NOT use function blocks or tool calls for these messages. |
There was a problem hiding this comment.
Again, not always good advice.
There was a problem hiding this comment.
defined TechnicalPossibilities
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
I'm a little concerned that golden files aren't the best way to test this (might be fragile), but we can assess that later and change it if they become annoying.
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
Goldens are great, because they are easy to fix, with visibility on PRs about what is changed. Text goldens are better than picture goldens, because they are not flacky. But, yes, some extra effort. Yes, let's see how it goes. |
2 participants
Contributes to #751