-
Notifications
You must be signed in to change notification settings - Fork 111
Switch from network images to asset images. #63
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully transitions the example app from using network images to local asset images, which is a great move for stability. The changes are logical, including updating the pubspec, adding assets, and modifying the code to load them. I've identified a critical runtime issue with variable initialization that will crash the app, a high-severity issue with a brittle JSON parsing method, and a medium-severity suggestion to improve the new test's robustness. Addressing these points will make the implementation solid.
jacobsimionato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey nice work! Do the images seem to load successfully from the assets?
| ); | ||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the concept of an ImageCatalog here is kind of unrelated to the idea of a catalog of widgets. So maybe this could live in a different file, e.g. test_images.dart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added '@VisibleForTesting' to the path constants.
However, imagesCatalogJson() is what is executed to provide images to prompt.
I would suggest that: UI catalog = widgets + images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the use of images and widgets here is quite different, because the widget catalog is something that developers will definitely replicate in a real app. The catalog of images here would not exist in a real app - this is just a test utility to give the LLM some images to use in the absence of "real" images which would be fetched from some tool that calls a database etc. That's what my hotel_database change was about - give the LLM a tool to fetch data, which would include image references (probably URLs).
So I like the idea of separating this into catalog.dart which developers can branch and modify, and something like test_images.dart or demo_images.dart which is clearly marked as a demo-only workaround that developers will not try to replicate in a real app. We don't want developers to think that they need to declare all images that their app can use in advance, because that won't be possible for a real use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I partially agree.
Yes, developers will need to provide some way to fetch images.
First, I think this 'some way' is part of the UI catalog definition. Do you think differently?
Second, I think, this 'some way' may be combination of:
- An MCP service (is it how we plan to shape the database?)
- Hard coded assets or network images (like company logo in travel app or small set of images of garden tools that the company sells)
- Some other way
At the moment we provide just #2, and we have plans to implement #1. We can add a note to 'USAGE.md', explaining that the database possibility is coming soon. And, I added prefix 'asset' to specify that there can be other options to provide images. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey I expect pretty much all images will come from tool calls, potentially using MCP to expose the tools or not. I expect company logos and other assets are more likely to be baked into the CatalogItems.
But this is just a first pass on the app and I don't want to keep getting in the way, so let's stick with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good to exchange opinions and align on mental models.
I see what you mean. Thank you for explanations.
Moved to asset_images.dart.
Yes, travel app works! |
| ); | ||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey I expect pretty much all images will come from tool calls, potentially using MCP to expose the tools or not. I expect company logos and other assets are more likely to be baked into the CatalogItems.
But this is just a first pass on the app and I don't want to keep getting in the way, so let's stick with this.
This is for stability of example.
Users may choose to work with their network images.