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

Fix: One-Time Secret Display for API Key (#771) #799

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

shreyanshVIT23
Copy link

@shreyanshVIT23 shreyanshVIT23 commented Feb 26, 2025

User description

Description

Implemented the One-Time Secret Display for API Keys.

Fixes #771

Dependencies

  • Uses shadcn/ui components for UI consistency.
  • Utilizes lucide-react icons for UI elements.

Future Improvements

  • Improve accessibility for screen readers.
  • Add an option to regenerate API keys if lost.

Mentions

@keyshade-xyz/team @maintainers


PR Type

Enhancement


Description

  • Added a new OneTimeSecretDisplay component to show API keys once.

  • Implemented UI elements for copying and toggling API key visibility.

  • Included warning message emphasizing one-time visibility of API keys.

  • Integrated dialog functionality with customizable open/close behavior.


Changes walkthrough 📝

Relevant files
Enhancement
index.tsx
Add `OneTimeSecretDisplay` component for API key                 

apps/platform/src/components/dashboard/secret/OneTimeSecretDisplay/index.tsx

  • Introduced OneTimeSecretDisplay component for API key display.
  • Added UI for copying and toggling API key visibility.
  • Included warning message for one-time API key visibility.
  • Implemented dialog with customizable open/close behavior.
  • +134/-0 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    771 - Partially compliant

    Compliant requirements:

    • Created dialog component to show API key value one time after creation
    • Component created under correct path (platform/src/components/dashboard/secret)

    Non-compliant requirements:

    • No explicit verification that implementation matches Figma design

    Requires further human verification:

    • Visual verification needed to ensure UI matches provided Figma design
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    State Management

    The component maintains duplicate state with both external open prop and internal internalOpen state. This could lead to synchronization issues.

    const [internalOpen, setInternalOpen] = useState(open)
    const [showSecret, setShowSecret] = useState(false)
    
    useEffect(() => {
      setInternalOpen(open)
    }, [open])
    Error Handling

    Clipboard operations can fail but there's no error handling in the handleCopy function.

    const handleCopy = async () => {
      await navigator.clipboard.writeText(secretValue)
      setCopied(true)
      setTimeout(() => setCopied(false), 2000)
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent accidental secret loss

    Add a confirmation step before closing the dialog when the API key hasn't been
    copied yet, to prevent accidental loss of access to the one-time secret.

    apps/platform/src/components/dashboard/secret/OneTimeSecretDisplay/index.tsx [37-40]

     const handleClose = () => {
    +  if (!copied && !showSecret) {
    +    const confirmClose = window.confirm('Are you sure you want to close? You won\'t be able to see this API key again.')
    +    if (!confirmClose) return
    +  }
       onOpenChange(false)
       setInternalOpen(false)
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: Adding a confirmation dialog before closing is crucial since this is a one-time secret that can't be retrieved again. This prevents accidental data loss and improves user experience.

    High
    • More

    @rajdip-b
    Copy link
    Member

    can you send in a screen recording for this?

    @shreyanshVIT23
    Copy link
    Author

    I am unable to run the code as i am currently facing an error. Please guide me as I am a newcomer in open source world.
    image

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Mar 1, 2025

    This is very odd. Are you sure your banch is updated?

    @kriptonian1
    Copy link
    Contributor

    @shreyanshVIT23 I think you haven't built the schemas, you need to use the command pnpm build:schema, let me know if this works for you or not

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Mar 7, 2025

    @shreyanshVIT23 any progress?

    @shreyanshVIT23
    Copy link
    Author

    Yes thankfully i have made some progress but now i am facing another issue. The error was because the prisma client wasn't generating for some reason so I had to manually set the schema.prisma location and run it. Now i am running the
    pnpm dev:platform
    but I am unable to get past the auth page

    https://github.com/user-attachments/assets/5ab4632e-3180-4bfb-99e4-8023d3b7617a
    image

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Mar 8, 2025

    @shreyanshVIT23 did you run the API? this error means that it couldn't reach the backend. Also make sure you have set NEXT_PUBLIC_BACKEND_URL=http://localhost:4200 in the .env file

    @rajdip-b
    Copy link
    Member

    @shreyanshVIT23 any updates?

    @shreyanshVIT23
    Copy link
    Author

    Yes! I am finally able to run the frontend thankfully. Keyshade docs with running locally section where huge help. I had to set up sentry account and SMTP which i didn't know about at all but the error messages where extremely helpful and https://www.youtube.com/watch?v=FFPHHXOlJC4&t=2236s where a huge help.
    image

    But I am unable to see any option to create api key is that something I must implement?

    @rajdip-b
    Copy link
    Member

    Update your fork with the latest code. It has the changes

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: Show the API key value for one time after it is created
    4 participants