OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

refactor(frontend): settings modal

Open amanape opened this issue 1 year ago • 1 comments

Summary

Rebuild the settings modal from the ground up adhering to React best practices and improve architectural robustness (frontend).

Motivation

My goal is to enhance the frontend codebase and improve its adaptability and extensibility, considering the project's early stage. I'm beginning with the simpler components to fully grasp the frontend's scope before progressing to the more complex components.

Fixes

  • ~~User can no longer enter a random string that is not included in the list of models and save it~~ Undone so user can enter custom values as previously

Improvements

  1. WELL TESTED (TDD for the majority/all of the new components)
  2. Less state management
  3. Improved <BaseModal> component (<LoadMessageModal> still uses previous <ODModal> for now)
  4. Consistent usage of <Autocomplete> for language selection rather than <Select> (more suitable when scaled)
  5. Easier to read and extend

Features

  • Loading spinner (it loads as long as it takes the agent to initialize; but thats for another issue)

Issues

  • Because of IMPROVEMENT#4 and the usage of the <Autocomplete> component, the input prefers a placeholder that does not exist (I18nKey.CONFIGURATION$LANGUAGE_SELECT_PLACEHOLDER in src/components/settings/AutocompleteCombobox.tsx).

Testing

Run npm test settings base-modal to run the relevant tests

amanape avatar Apr 20 '24 22:04 amanape

Be careful

User can no longer enter a random string that is not included in the list of models and save it

The backend that returns list(set(litellm.model_list + list(litellm.model_cost.keys()))) cannot completely cover all models. For instance, ollama/gemma:2b is not included in the list, yet it is a valid model.

Umpire2018 avatar Apr 21 '24 02:04 Umpire2018

Looks good! I think there's just a few CI checks failing..

rbren avatar Apr 21 '24 19:04 rbren

@rbren Those checks are regarding the i18n placeholder value for the language input which does not exist. Previously, the settings modal component was using a <Select> and was not set.

Because of reusability, each input now expects similar values, including placeholder. The component does not fail if a placeholder is not given, but I left it as is to grab the attention of a translator.

I see now the CI tests fail because of the error, so I will just add the new translations with the help of AI. Not sure what the standard approach for this scenario is.

amanape avatar Apr 21 '24 19:04 amanape