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(platform): Delete variable from a project #600

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

Conversation

poswalsameer
Copy link
Contributor

@poswalsameer poswalsameer commented Dec 29, 2024

User description

Description

This PR adds the feature of deleting a variable from a project.

Fixes #560

Mentions

@kriptonian1

Screenshots of relevant screens

Screenshot 2024-12-30 015940
Screenshot 2024-12-30 015948

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, Documentation


Description

This PR implements the variable management feature in the project, including:

  • Added ability to create and delete variables within a project
  • Implemented variable listing with collapsible details showing environment values
  • Added confirmation dialog for variable deletion with proper error handling
  • Enhanced UI components:
    • New reusable Textarea component
    • Alert dialog for confirmations
    • Improved project creation form
  • Fixed type safety issues in API response parsing
  • Updated project navigation to use slugs instead of IDs
  • Added new SVG icons for improved UI
  • Implemented proper success/error toasts for user feedback

Changes walkthrough 📝

Relevant files
Enhancement
8 files
layout.tsx
Add variable creation and management functionality             
+294/-52
page.tsx
Implement variable listing and deletion feature                   
+249/-4 
alert-dialog.tsx
Add reusable alert dialog component                                           
+141/-0 
page.tsx
Update project creation form with textarea                             
+25/-12 
confirm-delete.tsx
Add confirmation dialog for variable deletion                       
+103/-0 
textarea.tsx
Add reusable textarea component                                                   
+22/-0   
index.tsx
Update project card link to use slug                                         
+2/-1     
index.ts
Add new SVG icon exports                                                                 
+9/-1     
Bug fix
1 files
response-parser.ts
Fix type casting in response parser                                           
+2/-2     
Additional files
4 files
collapsible.tsx +12/-0   
erDetails(userData) +0/-664 
t updateSelf = useCallback(async () => { +0/-664 

💡 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 ✅

560 - PR Code Verified

Compliant requirements:

  • Implement variable deletion functionality for projects
  • Follow Figma design for UI implementation
  • Implement in apps/platform/src/app/(main)/project/[project]/@variable/page.tsx
  • Integrate with Bruno API for delete variable functionality

Requires further human verification:

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

Error Handling

The error handling in deleteVariable function only logs errors to console without showing user feedback, unlike success case which shows a toast. Consider showing error feedback to users.

if( error ){
  console.error(error)
}
State Management

The useEffect hook has allVariables in its dependency array which could cause infinite re-renders since allVariables is being set within the effect itself.

useEffect(() => {
  const getAllVariables = async () => {
    if (!currentProject) {
      return
    }

    const {
      success,
      error,
      data
    }: ClientResponse<GetAllVariablesOfProjectResponse> =
      await ControllerInstance.getInstance().variableController.getAllVariablesOfProject(
        { projectSlug: currentProject.slug },
        {}
      )

    if (success && data) {
      setAllVariables(data.items)
    } else {
      // eslint-disable-next-line no-console -- we need to log the error
      console.error(error)
    }
  }

  getAllVariables()
}, [currentProject, allVariables])

Copy link
Contributor

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

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
✅ Fix reference to undefined variable in useEffect dependency array

Fix the undefined 'open' variable usage in useEffect which could cause runtime
errors.

apps/platform/src/components/ui/confirm-delete.tsx [64-68]

 useEffect(() => {
-  if (!open) {
+  if (!isOpen) {
     cleanup()
   }
   return () => cleanup()
-}, [open, cleanup])
+}, [isOpen, cleanup])

[Suggestion has been applied]

Suggestion importance[1-10]: 9

Why: The suggestion fixes a critical bug where an undefined 'open' variable is used instead of the correct 'isOpen' prop, which could cause runtime errors.

9
Add proper validation and error handling for missing variable slugs before deletion

Add input validation before submitting the variable deletion request to prevent
potential errors from null/undefined variable slugs.

apps/platform/src/app/(main)/project/[project]/@variable/page.tsx [28-33]

 const deleteVariable = async () => {
-  if( variableSlug === null ){
+  if (!variableSlug) {
+    toast.error('Invalid variable selected', {
+      description: 'Cannot delete variable: No variable was selected'
+    })
+    onClose()
     return
   }
   const { success, error } = await ControllerInstance.getInstance().variableController.deleteVariable(
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion adds important input validation with user feedback for null/undefined variable slugs, preventing potential runtime errors and improving error handling.

7
General
✅ Add user-facing error notifications when variable deletion fails instead of only logging to console

Add error handling for the case when variable deletion fails. Currently, errors are
only logged to console, but users should be notified when deletion fails.

apps/platform/src/app/(main)/project/[project]/@variable/page.tsx [40-52]

 if (success) {
   toast.success('Variable deleted successfully', {
     description: () => (
       <p className="text-xs text-emerald-300">
         The variable has been deleted.
       </p>
     )
   })
 }
-if( error ){
+if (error) {
+  toast.error('Failed to delete variable', {
+    description: () => (
+      <p className="text-xs text-red-300">
+        An error occurred while deleting the variable. Please try again.
+      </p>
+    )
+  })
   console.error(error)
 }

[Suggestion has been applied]

Suggestion importance[1-10]: 8

Why: The suggestion improves error handling by adding user-facing error notifications instead of just console logging, which significantly enhances user experience by providing clear feedback when operations fail.

8

@poswalsameer poswalsameer changed the title feat: Delete variable from a project feat(platform): Delete variable from a project Dec 30, 2024
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.

PLATFROM: Delete variable from a project
2 participants