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.

Sign up for GitHub

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

Jump to bottom

fix: fixed ui-ux for api-key-manager component #732

Merged
merged 7 commits into from
Jan 10, 2025
Merged

fix: fixed ui-ux for api-key-manager component #732

merged 7 commits into from
Jan 10, 2025

Conversation

Adithyan777
Copy link

  • Modified the API Key Manager to show users whether the API key is set via environment variables or the UI.
  • Implemented an API route to check and report the API key status dynamically based on the model provider.
  • The API key set via the UI is reset when the user switches to a different model provider

Screenshot 2024-12-15 at 1 53 33 PM
Screenshot 2024-12-15 at 1 53 55 PM
Screenshot 2024-12-15 at 1 58 30 PM
Screenshot 2024-12-15 at 2 03 29 PM

@Adithyan777 Adithyan777 changed the title ui-ux : improved api-key-manager component ui-ux : fixed api-key-manager component Dec 16, 2024
@kalyanipujari204
Copy link

bro can you tell me step by step how to run this project on vs code

thecodacus
thecodacus requested changes Dec 19, 2024
View reviewed changes
Copy link
Collaborator

@thecodacus thecodacus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a great implementation, it keeps the keys safe and gives feedback.
I have added some comments please check those, after that its good to merge

app/components/chat/APIKeyManager.tsx Outdated Show resolved Hide resolved
app/components/chat/APIKeyManager.tsx Outdated Show resolved Hide resolved
app/routes/api.check-env-key.ts Outdated Show resolved Hide resolved
app/routes/api.check-env-key.ts Show resolved Hide resolved
@thecodacus thecodacus mentioned this pull request Dec 19, 2024
Closed
@thecodacus
Copy link
Collaborator

hi @Adithyan777, I want to merge this for the next release, will you be addressing the changes I have mentioned in the review, so that i can queue up this for next release?

@thecodacus thecodacus added this to the v0.0.4 milestone Dec 30, 2024
@Adithyan777
Copy link
Author

hi @Adithyan777, I want to merge this for the next release, will you be addressing the changes I have mentioned in the review, so that i can queue up this for next release?

hi @thecodacus , I apologize for the delay in addressing the requested changes. I’m currently traveling and won’t have access to my computer for the next 3-4 days. Once I’m back, I’ll prioritize implementing the changes and notify you as soon as they are ready.

@thecodacus
Copy link
Collaborator

sure, thanks for the update

@thecodacus thecodacus modified the milestones: v0.0.4, v0.0.5 Dec 31, 2024
@Adithyan777
Merge remote-tracking branch 'upstream/main' into feat/improve-api-ke…
987fd35 
…y-manager-ui
@Adithyan777
addressed some of the review changes:
a799dd6 
1. moved function definiton to useCallback.
2. added a cache to store the env key status and the api call is made only on a cache miss.
@Adithyan777
Manages the API-key entered via UI in a better way.
4163868 
- Persist API keys in cookies when entered via UI
- Automatically load saved keys when switching between providers
- Preserve existing functionality for environment variable based keys
@Adithyan777
Copy link
Author

hi @thecodacus, I've addressed most of the requested changes from the review. However, for the suggestion to import the map from utils/constants file , I couldn't locate it. Could you confirm if it's defined elsewhere, or provide more details on where I can find it?

@Adithyan777 Adithyan777 requested a review from thecodacus January 4, 2025 20:18
@thecodacus
Copy link
Collaborator

Hi, thanks for the update,
you can check this variable from constants which has map of all the env keys names

image

@Adithyan777
Copy link
Author

hi @thecodacus , I've addressed all the requested changes. Let me know if there's anything else to change.

@thecodacus
Copy link
Collaborator

hi @thecodacus , I've addressed all the requested changes. Let me know if there's anything else to change.

everything looks good except the apikey set on provider change might be redundant block.
I have added comment.
once this addressed I can merge this

@thecodacus thecodacus changed the title ui-ux : fixed api-key-manager component fix : fixed ui-ux for api-key-manager component Jan 6, 2025
@Adithyan777 Adithyan777 requested a review from thecodacus January 6, 2025 17:02
@thecodacus thecodacus changed the title fix : fixed ui-ux for api-key-manager component fix: fixed ui-ux for api-key-manager component Jan 6, 2025
@thecodacus thecodacus merged commit 49bb178 into stackblitz-labs:main Jan 10, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thecodacus thecodacus thecodacus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.0.6
Development

Successfully merging this pull request may close these issues.
3 participants
@Adithyan777 @kalyanipujari204 @thecodacus