-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[BUG FIX] Remove usage of mapProvider and supporting tests due to undesired int… #8972
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
base: main
Are you sure you want to change the base?
[BUG FIX] Remove usage of mapProvider and supporting tests due to undesired int… #8972
Conversation
…eraction with HeroUI's autocomplete behavior
openai: "OpenAI", | ||
azure: "Azure", | ||
azure_ai: "Azure AI Studio", | ||
vertex_ai: "VertexAI", |
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'm a bit surprised by this solution, I mean deleting all map provider. Are we sure, it seems to work for everything else. Could you please add in the PR description or here what exactly doesn't work for Vertex, so we can ping our FE master and see if maybe there's another cause?
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.
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.
Response: https://www.loom.com/share/e8d853dff1874cfa8a71cfac87980607?sid=7af74516-b646-4cd5-90c6-cb4528cfd3ab
ChatGPT's Summary of Video Transcript
Totally fair to be surprised - I was honestly torn myself. Here's the thinking:
There are only two references to MapProvider in main, and it's being used solely for styling the display of the underlying value. Functionally, it’s not doing much.
The main driver behind this change is that Autocomplete in Hero UI is meant to behave like an input, not a select. So sending the display text (label) on submission aligns with how it’s intended to work. They even show an example using allowCustomValue, which supports this approach - but it also means users can type anything, so there's some tradeoff there.
Using a select instead would force you into predefined options but lose the ability to type-to-search, which isn’t ideal for this case.
So my decision leaned toward keeping things simpler and making the input reflect exactly what gets passed to LightLLM. This approach reduces complexity and better aligns with the expected behavior for future devs.
That said, if we want to go full controlled component for the sake of styling or interaction fidelity, happy to do that too - just didn’t feel like the right tradeoff here. Let me know
Following up on this - repeating the wins:
Cost: Loss of a pretty label. |
This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
End-user friendly description of the problem this fixes or functionality this introduces.
Fixes an issue where the model provider selection (e.g., Vertex AI) in the HeroUI autocomplete component did not correctly persist the selected value. Previously, typing and selecting an option would submit an invalid provider identifier (e.g., missing underscores), causing LightLLM to fail during startup. This change ensures that the selected value is accurately saved and used, eliminating hidden UI mismatches and improving reliability when configuring providers.
Video recording: https://www.loom.com/share/7a95d9fd23e246e7ab44376ab3bf4783?sid=e3ab1673-f86b-4c3d-88bb-a35e97208040
Relevant Herou UI Links:
heroui-inc/heroui#3353
heroui-inc/heroui#3375 (comment)
Summarize what the PR does, explaining any non-trivial design decisions.
Removes usage of mapProvider and associated tests from the frontend.
Link of any specific issues this addresses:
Resolves #8689