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

feat(platfrom): Add new variables to a project #593

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

poswalsameer
Copy link
Contributor

@poswalsameer poswalsameer commented Dec 20, 2024

User description

Description

This PR adds the feature to add new variables to a project.

Fixes #559

Screenshots of relevant screens

Screenshot 2024-12-11 160310
Screenshot (617)

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement


Description

  • Added new variable creation functionality with a styled dialog form
  • Implemented environment selection with dynamic loading of available environments
  • Added form fields for variable name, note, environment name and value
  • Integrated with API for creating new variables
  • Updated project card navigation to use slugs instead of IDs
  • Enhanced UI with proper styling and form validation
  • Added proper error handling for API calls

Changes walkthrough 📝

Relevant files
Enhancement
layout.tsx
Add new variable creation dialog and functionality             

apps/platform/src/app/(main)/project/[project]/layout.tsx

  • Added new variable creation functionality with form dialog
  • Implemented environment selection and value input fields
  • Added API integration for creating variables
  • Enhanced UI with styled components and proper form validation
  • +247/-48
    index.tsx
    Update project card navigation to use slugs                           

    apps/platform/src/components/dashboard/projectCard/index.tsx

  • Updated project link to use slug instead of id
  • Added slug to project card props
  • +2/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    559 - Fully compliant

    Compliant requirements:

    • Added popup dialog for new variable creation
    • UI matches Figma design with proper styling
    • Feature implemented in correct file location
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The error handling in addVariable function only logs to console without showing user feedback or retrying failed requests

        if(error){
          // eslint-disable-next-line no-console -- we need to log the error
          console.error(error)
        }
    Form Validation

    No validation is performed on the variable name and value fields before submission, which could lead to invalid data being sent to API

                        <Input
                          id="variable-name"
                          placeholder="Enter the key of the variable"
                          value={newVariableData.variableName}
                          onChange={(e) =>
                            setNewVariableData({
                              ...newVariableData,
                              variableName: e.target.value
                            })
                          }
                          className="h-[2.75rem] w-[20rem] border-0 bg-[#2a2a2a] text-gray-300 placeholder:text-gray-500"
                        />

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Dec 20, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add form validation to prevent submission of invalid data
    Suggestion Impact:The commit adds validation for currentProject existence, though in a different way than suggested. It throws an error if currentProject doesn't exist rather than showing a toast message.

    code diff:

    +    if (!currentProject) {
    +      throw new Error("Current project doesn't exist")
    +    }

    Add input validation before submitting the variable creation request. Currently the
    form can be submitted with empty required fields which could cause API errors.

    apps/platform/src/app/(main)/project/[project]/layout.tsx [60-65]

    -const addVariable = async (e: any) => {
    +const addVariable = async (e: React.FormEvent) => {
       e.preventDefault()
    +  if (!newVariableData.variableName.trim()) {
    +    toast.error('Variable name is required');
    +    return;
    +  }
    +  if (!currentProject?.slug) {
    +    toast.error('Project not found');
    +    return;
    +  }
       const request: CreateVariableRequest = {
    -    name: newVariableData.variableName,
    -    projectSlug: currentProject?.slug as string,
    +    name: newVariableData.variableName.trim(),
    +    projectSlug: currentProject.slug,
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding input validation is critical for preventing API errors and ensuring data integrity. This helps avoid runtime errors and provides better user feedback for required fields.

    9
    ✅ Replace type suppression with proper type assertion to maintain type safety
    Suggestion Impact:The @ts-ignore comment was removed from the setCurrentProject call, improving type safety

    code diff:

    -        //@ts-ignore
             setCurrentProject(data)

    Remove the @ts-ignore comment and properly type the response data from getProject
    API call. Using @ts-ignore masks potential type mismatches that could cause runtime
    errors.

    apps/platform/src/app/(main)/project/[project]/layout.tsx [99-100]

     if (success && data) {
    -  //@ts-ignore
    -  setCurrentProject(data)
    +  setCurrentProject(data as Project)
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Removing @ts-ignore and using proper type assertion improves type safety and helps catch potential runtime errors during development. This is important for maintaining code reliability.

    7
    General
    ✅ Enhance error handling with user-facing notifications instead of just console logging
    Suggestion Impact:The commit implemented toast notifications for both success and error cases when creating variables, including specific error handling for 409 conflict errors

    code diff:

    +    if (success) {
    +      toast.success('Variable added successfully', {
    +        // eslint-disable-next-line react/no-unstable-nested-components -- we need to nest the description
    +        description: () => (
    +          <p className="text-xs text-emerald-300">
    +            The variable has been added to the project
    +          </p>
    +        )
    +      })
    +    }
    +
    +    if (error) {
    +      if (error.statusCode === 409) {
    +        toast.error('Variable name already exists', {
    +          // eslint-disable-next-line react/no-unstable-nested-components -- we need to nest the description
    +          description: () => (
    +            <p className="text-xs text-red-300">
    +              Variable name is already there, kindly use different one.
    +            </p>
    +          )
    +        })
    +      } else {
    +        // eslint-disable-next-line no-console -- we need to log the error that are not in the if condition
    +        console.error(error)
    +      }
         }

    Add proper error handling and user feedback for the variable creation API call.
    Currently errors are only logged to console which provides poor UX. Consider showing
    a toast/notification to inform users of success/failure.

    apps/platform/src/app/(main)/project/[project]/layout.tsx [82-85]

    -if(error){
    -  // eslint-disable-next-line no-console -- we need to log the error
    -  console.error(error)
    +if (error) {
    +  toast.error('Failed to create variable: ' + error.message);
    +  return;
     }
    +toast.success('Variable created successfully');
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding user-facing error notifications significantly improves UX by providing clear feedback on operation success/failure, rather than silently logging to console which users can't see.

    8

    @kriptonian1 kriptonian1 changed the title feat: Add new variables to a project feat(platfrom): Add new variables to a project Dec 21, 2024
    @rajdip-b
    Copy link
    Member

    @kriptonian1 review ser

    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a comment

    Choose a reason for hiding this comment

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

    LGTM 🚀

    @kriptonian1
    Copy link
    Contributor

    @rajdip-b you can merge this code now

    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: Add new variable in a project
    3 participants