Skip to content
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

434 edit delete assistants #510

Merged
merged 107 commits into from
May 20, 2024
Merged

434 edit delete assistants #510

merged 107 commits into from
May 20, 2024

Conversation

andrewrisse
Copy link
Contributor

@andrewrisse andrewrisse commented May 14, 2024

closes #434
closes #480

Features:

  1. Edit Assistants
  2. Delete Assistants
  3. New form validation realtime feedback
  4. Update avatar cancel modal to only show on navigation if form is tainted

Chores:

  1. Refactors assistants to use sveltekit-superforms library. Enables cleaner form logic on client and server, as well as progressive enhancement features including realtime field validation
  2. New pattern for mocking Supabase in unit tests. Not all tests have been refactored to use this yet
  3. Adds additional unit tests for new/edit assistant form actions

Notes:

  1. No unit tests for delete assistant endpoint as this functionality will be moving to the Leapfrog API soon
  2. When deploying, the new migration for assistants storage must be applied. This feature also changes the way we store assistants (stored by assistant id, instead of under user uid). Any avatars previously uploaded will no longer be visible and will need to be edited.

PR is deployed on LF-04: https://ai.uds.dev/

@andrewrisse
Copy link
Contributor Author

andrewrisse commented May 15, 2024

Thanks again for recording the video! At 1:27 in the recording, when you click on the Assistant Name field, a drop-down briefly appears, which looks like it's pulling users from Keycloak. (If I'm interpreting that wrong, let me know.) Why does that make sense to appear when editing the Assistant Name?

I would think the Assistant Name is a free-form text field that you can populate with anything, e.g., "Dougbot AFI Expert" if I was making an assistant with access to lots of AFIs. I don't understand why that drop-down list appears when selecting the field.

Along those lines of not understanding... it looks like all of the other field name headers have a little (i) tooltip icons next to them EXCEPT for the top "Name" field. If that Name field is supposed to be somehow associated with the user then it probably deserves a little (i) tooltip explanation too.

@barronstone , that dropdown is actually pulling my 1Password autofill suggestions. HTML inputs are give "name" attributes, for example name="email" or name="password". Auto-fill looks for these names to suggest things to fill it with, that's why when you start typing in an "email" field on a website, it auto-suggests your email addresses.

The specific field you saw in the video has name="name" because the Assistant object has a "name" filed in it so 1password suggested some names for me.

I can try turning off autofill on that field.

@gregclark-defenseunicorns can chime in if he wants to add a tooltip to this field like the others. Usually tooltips are only used on fields that are self explanatory and I think "Name" is pretty straight forward. Maybe we could label the field "Assistant Name" to be more clear?

@barronstone
Copy link
Collaborator

Thanks again for recording the video! At 1:27 in the recording, when you click on the Assistant Name field, a drop-down briefly appears, which looks like it's pulling users from Keycloak. (If I'm interpreting that wrong, let me know.) Why does that make sense to appear when editing the Assistant Name?
I would think the Assistant Name is a free-form text field that you can populate with anything, e.g., "Dougbot AFI Expert" if I was making an assistant with access to lots of AFIs. I don't understand why that drop-down list appears when selecting the field.
Along those lines of not understanding... it looks like all of the other field name headers have a little (i) tooltip icons next to them EXCEPT for the top "Name" field. If that Name field is supposed to be somehow associated with the user then it probably deserves a little (i) tooltip explanation too.

@barronstone , that dropdown is actually pulling my 1Password autofill suggestions. HTML inputs are give "name" attributes, for example name="email" or name="password". Auto-fill looks for these names to suggest things to fill it with, that's why when you start typing in an "email" field on a website, it auto-suggests your email addresses.

The specific field you saw in the video has name="name" because the Assistant object has a "name" filed in it so 1password suggested some names for me.

I can try turning off autofill on that field.

@gregclark-defenseunicorns can chime in if he wants to add a tooltip to this field like the others. Usually tooltips are only used on fields that are self explanatory and I think "Name" is pretty straight forward. Maybe we could label the field "Assistant Name" to be more clear?

@andrewrisse Now that I understand what was happening in that video with 1Password auto-filling, I don't think there's a need for a tooltip. Name should be self-explanatory enough in that context.

@barronstone barronstone reopened this May 15, 2024
Copy link
Contributor

@gregclark-defenseunicorns gregclark-defenseunicorns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design review passed
Will fix this bug in a separate issue

@YrrepNoj YrrepNoj merged commit e8408b7 into main May 20, 2024
9 checks passed
@YrrepNoj YrrepNoj deleted the 434-edit-delete-assistants branch May 20, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(ui) 3 dot menu and delete assistant flow feat(ui) Edit Assistant
7 participants