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: added delete variable loader #752

Closed

Conversation

chetan102
Copy link

@chetan102 chetan102 commented Feb 12, 2025

User description

#714 Added Loader to the delete variable loader


PR Type

Enhancement


Description

  • Added a loading state when deleting a variable.

  • Improved user feedback with toast messages for success and error states.

  • Disabled the delete button during ongoing requests to prevent multiple submissions.

  • Ensured proper cleanup and dismissal of toast notifications after completion.


Changes walkthrough 📝

Relevant files
Enhancement
index.tsx
Add loading state and improve feedback for variable deletion

apps/platform/src/components/dashboard/variable/confirmDeleteVariable/index.tsx

  • Added toast.loading to indicate the deletion process.
  • Wrapped the deletion logic in a try block for better error handling.
  • Used toast.dismiss in the finally block to ensure cleanup.
  • Enhanced user feedback with toast.success and toast.error messages.
  • +34/-30 

    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 🔶

    714 - Partially compliant

    Compliant requirements:

    • Add loading state when deleting a variable
    • Handle request flow with toast messages (loading, success, error)
    • Show proper toast messages for feedback

    Non-compliant requirements:

    • Disable multiple requests while one is ongoing

    Requires further human verification:

    • Verify that the button is actually disabled during deletion process
    • Verify toast messages appear and disappear correctly in the UI
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Race Condition

    The code does not prevent multiple delete requests from being triggered simultaneously. While a loading toast is shown, there's no mechanism to disable the delete functionality while a request is in progress.

    toast.loading('Deleting Variable...')
    const variableSlug = selectedVariable.variable.slug
    
    try {
      const { success, error } =
        await ControllerInstance.getInstance().variableController.deleteVariable(
          { variableSlug },
          {}
        )
    
      if (success) {
        toast.success('Variable deleted successfully', {
          description: (
            <p className="text-xs text-emerald-300">
              The variable has been deleted.
            </p>
          )
        })
    
        // Remove the variable from the store
        setVariables((prevVariables) =>
          prevVariables.filter(({ variable }) => variable.slug !== variableSlug)
        )
      }
      if (error) {
        toast.error('Something went wrong!', {
          description: (
            <p className="text-xs text-red-300">
              Something went wrong while deleting the variable. Check console
              for more info.
            </p>
          )
        })
        // eslint-disable-next-line no-console -- we need to log the error
        console.error(error)
      }
    } finally {
      toast.dismiss()
      handleClose()
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add API error handling

    Add error handling for the API call itself by catching network/API errors
    separately from business logic errors. Currently, API failures might not be
    properly caught.

    apps/platform/src/components/dashboard/variable/confirmDeleteVariable/index.tsx [50-55]

     try {
    -  const { success, error } =
    -    await ControllerInstance.getInstance().variableController.deleteVariable(
    -      { variableSlug },
    -      {}
    -    )
    +  const { success, error } = await ControllerInstance.getInstance()
    +    .variableController.deleteVariable({ variableSlug }, {})
    +    .catch((apiError) => {
    +      console.error('API Error:', apiError);
    +      return { success: false, error: apiError };
    +    });
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a critical gap in error handling by catching potential network/API errors that could crash the application. This is particularly important for API calls where various network-related errors could occur.

    Medium
    General
    Improve error handling and cleanup

    The loading toast might stay visible if an uncaught exception occurs. Move
    toast.dismiss() to a separate finally block after the try-catch.

    apps/platform/src/components/dashboard/variable/confirmDeleteVariable/index.tsx [50-86]

     try {
       const { success, error } =
         await ControllerInstance.getInstance().variableController.deleteVariable(
           { variableSlug },
           {}
         )
       // ... rest of the code ...
    +} catch (error) {
    +  console.error('Unexpected error:', error);
    +  toast.error('An unexpected error occurred');
     } finally {
       toast.dismiss()
       handleClose()
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds a catch block for unexpected errors and ensures proper cleanup, which is important for maintaining a good user experience and preventing the loading state from being stuck.

    Medium

    @chetan102 chetan102 closed this Feb 12, 2025
    @chetan102 chetan102 deleted the delete-variable-loader branch February 23, 2025 09:49
    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.

    1 participant